add CI job for running the unit tests#3513
Conversation
MoralCode
added
the
ready
|
Having automated CI testing in place is helpful for many of my future plans, including writing unit tests to confirm database leaks, getting more of the tests working, adding new tests, etc. |
MoralCode
removed
the
ready
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
using pytest avoids two layers of python environment "the gap between task runners like tox and test runners like pytest is narrower now" - Gemini Signed-off-by: Adrian Edwards <adredwar@redhat.com>
MoralCode
force-pushed
the
ntdn/functional-tests
branch
from
da052c0 to
0fb462a
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| assert JsonConfig({"A": {}}, mock_logger).empty is False | ||
|
|
||
|
|
||
| def test_jsonconfig_write_protection(mock_logger): |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0621: Redefining name 'mock_logger' from outer scope (line 9) (redefined-outer-name)
| config_test = cfg.retrieve_dict() | ||
| assert config_test != data # the data in the config should not change | ||
|
|
||
| def test_jsonconfig_retrieve_has_get(mock_logger): |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0621: Redefining name 'mock_logger' from outer scope (line 9) (redefined-outer-name)
Signed-off-by: Adrian Edwards <17362949+MoralCode@users.noreply.github.com>
MoralCode
added
the
ready
shlokgilda
left a comment
shlokgilda
left a comment
There was a problem hiding this comment.
Looks good overall. Maybe just consider adding timeout-minutes to prevent hung jobs?
Good to merge even if timeout is not addressed (I think).
|
|
||
| def __init__(self, json_data, logger: logging.Logger): | ||
| super().__init__(logger) | ||
| if not self.writable: |
There was a problem hiding this comment.
Since writable is hardcoded to False for JsonConfig (line 554), this condition is always true. Not wrong, but could be simplified to just always deepcopy.
Up to you though - the current form is more defensive if writable ever becomes dynamic.
There was a problem hiding this comment.
Yeah i think im anticipating the possibility of something writable since I really want to move away from having config in a database in the long term. I just think we maybe need a better backend for it (maybe a config oriented version of keyman?) that isnt just an in-memory dict (too volatile) or file on disk (no way to deal with conflicts between what the user wants and what augur is trying to set)
Signed-off-by: Adrian Edwards <17362949+MoralCode@users.noreply.github.com>
MoralCode
added
the
high priority
|
waiting for the docker smoke test and will probably merge without a second review given the low impact (i.e. this is new stuff, not touching old code) of this PR and the relatively high priority (other stuff is waiting on this) |
2 participants
Description
This adds a github CI job that runs the unit tests. This job is in a new file called functional_test because the unit tests that are currently enable test functionality that is critical to augurs functioning, so if they fail, the PR is substantially likely to contain broken code.
This Job is configured to run on each of the following minor python releases:
3.12(disabled. planning to merge fix(#3519): update tensorflow and dependencies to support python 3.12+ #3534 afterwards to get this working)3.13(will enable in future PR)3.14(will enable in future)These tests are also repeated on macOS and Ubuntu runners.
This is - as far as im aware - all the versions of python Augur currently supports.
This behavior was originally part of #3422 but has been moved out so that the failures can be dealt with in smaller groups
Currently the only behavior being unit tested here is for the Hierarchical config system, but I anticipate adding more in the future
Notes for Reviewers
This job will run when this PR is proposed. if the CI is passing, this PR should be considered "tested" and working
Signed commits