Skip to content

test_repository_tool: Add missing clear_all flag #1067

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

jku
Copy link
Member

@jku jku commented Jul 6, 2020

Without this the next create_*db() call will fail meaning
pytest test_repository_tool.py
fails on every test after this one.

Signed-off-by: Jussi Kukkonen [email protected]

@joshuagl
Copy link
Member

joshuagl commented Jul 6, 2020

Thanks for the patch @jku ! I am not familiar with pytest, so I'm not sure why this is required.

The setUp() method, which should be called at the start of each test, is already invoking clear_*db(clear_all=True). AIUI that should handle things not being correctly cleaned up in tearDown() (though maybe cleaning up correctly is the better way to do things, and we should remove those calls from setUp()?

The tests do work in our CI and if I invoke locally from within the tests subdirectory with python3 -m unittest -q test_repository_tool.

Should pytest work with standard library unittests? OOI why use pytest and not tox?

@jku
Copy link
Member Author

jku commented Jul 10, 2020

Sorting in order of importance

The tests do work in our CI and if I invoke locally from within the tests subdirectory with python3 -m unittest -q test_repository_tool.

Should pytest work with standard library unittests?

I have no idea because I thought they were the same thing: they don't seem to be. So this is probably just me not knowing about python testing and not a bug.

why use pytest and not tox?

To the question "why not use tox": the tox setup seems a little broken to me as there's no way to run a single test because the default environment runs coverage... I've tried to write a new environment config for that but it becomes a little clunky. Calling "python3 -m unittest" seems fine to me: I just didn't know it

The setUp() method, which should be called at the start of each test, is already invoking clear_*db(clear_all=True). AIUI that should handle things not being correctly cleaned up in tearDown() (though maybe cleaning up correctly is the better way to do things, and we should remove those calls from setUp()?

This is only true for one of the classes I think: the others do not invoke the clear functions in setUp() at all -- this one was the only class that does not use clear_all out of all the classes so I figured that was the issue.

@jku
Copy link
Member Author

jku commented Jul 10, 2020

Actually I think it is a real bug: if you rename TestTimestamp so it isn't the last test unittest runs, your command fails as well

@lukpueh
Copy link
Member

lukpueh commented Jul 13, 2020

To the question "why not use tox": the tox setup seems a little broken to me as there's no way to run a single test because the default environment runs coverage... I've tried to write a new environment config for that but it becomes a little clunky. Calling "python3 -m unittest" seems fine to me: I just didn't know it

FYI, you can run unittest-powerd modules/classes/tests directly like so:

python test_repository_tool.py # Runs all test cases (classes) in that module
python test_repository_tool.py TestRepository # Runs all tests (functions) in TestRepository test case
python test_repository_tool.py TestRepository.test_writeall # Runs test_writeall test only

Not sure if we can configure tox to allow this fine-grained control, or if that is actually necessary. On my box it would definitely take too long to create a fresh virtual environment and install everything with tox each time I want to run a specific test function. So I usually just run it directly in my dev virtual environment, which uses a recent Python version and has everything installed. I only run tox before I push.

@lukpueh
Copy link
Member

lukpueh commented Jul 13, 2020

@jku, do you think it would be helpful to have some of above info in our contributor testing docs?

@jku
Copy link
Member Author

jku commented Jul 13, 2020

@jku, do you think it would be helpful to have some of above info in our contributor testing docs?

They seem obvious in hindsight ...but yes: I was following those instructions and did not figure out how to tweak them to run a single test case.

Currently TestTimestamp creates custom databases but only clears the
default ones. This means next create_*db() call will fail meaning every
test after this one will fail (currently TestTimestamp happens to be
last but the effect can be seen by renaming it to TestATimestamp).

Also remove the clear_*db() calls from TestRepository::Setup(): they are
likely to be a workaround for a similar problem earlier (earlier test
failed to cleanup).

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku force-pushed the test-repository-tool-add-missing-clear-all branch from f0276ca to 9db013a Compare July 13, 2020 14:33
@jku
Copy link
Member Author

jku commented Jul 13, 2020

wrote a better commit message (without the pytest confusion), also removed the superfluous cleanup Joshua noticed -- I think that's probably a workaround of a similar issue earlier in history.

Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix, the discussion and the cleaned up commit message.

@joshuagl joshuagl merged commit 00e15c4 into theupdateframework:develop Jul 13, 2020
@jku jku deleted the test-repository-tool-add-missing-clear-all branch December 30, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants