Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix a test that only passed due to test interactions #443

Merged
merged 4 commits into from
Jul 28, 2021
Merged

Fix a test that only passed due to test interactions #443

merged 4 commits into from
Jul 28, 2021

Conversation

mdickinson
Copy link
Member

Fix a test that was only passing as a result of side-effects from other tests in the same TestCase.

The test was passing when run as part of the complete test case because a distribution of acme.foo with version 0.1a1 was already in the global working set. This PR changes the test to explicitly add acme.foo to the global working set, so that the test passes both standalone and as part of the whole test case.

(Note that the global working set is reset at tearDown time, though I plan another PR to clean up the test side-effects.)

Closes #441

mdickinson
mdickinson commented Jul 28, 2021
View reviewed changes
envisage/tests/test_egg_basket_plugin_manager.py
plugin_manager = EggBasketPluginManager(
plugin_path=[
self.bad_eggs_dir,
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fairly sure that we don't need the bad_eggs_dir for this test.

@mdickinson mdickinson mentioned this pull request Jul 28, 2021
Closed
@mdickinson mdickinson marked this pull request as draft July 28, 2021 08:08
@mdickinson
Copy link
Member Author

test_ignore_broken_distributions_loads_good_distributions has a similar-looking issue. Converting to draft while I figure out whether I can fix that test too as part of this PR.

@mdickinson
Copy link
Member Author

Second test fixed, and PR updated to use a cleaner fix for restoring sys.path.

@mdickinson mdickinson marked this pull request as ready for review July 28, 2021 08:15
rahulporuri
rahulporuri previously approved these changes Jul 28, 2021
View reviewed changes
@rahulporuri rahulporuri self-requested a review July 28, 2021 08:34
@rahulporuri
Copy link
Contributor

Never mind the approval. Didn't notice the CI failure.

@mdickinson
Copy link
Member Author

Didn't notice the CI failure.

Sigh. Neither did I. Investigating.

@mdickinson
Copy link
Member Author

Okay, looks like the problem is yet more dependence on global state. When I was testing, the acme.foo egg sitting inside the repository had been built, so it had an acme.foo.egg-info. After a git clean and a re-install, I can reproduce the failures locally.

@mdickinson mdickinson dismissed rahulporuri’s stale review July 28, 2021 09:26

Review was based on now-outdated code.

@mdickinson mdickinson merged commit afeda5c into master Jul 28, 2021
@mdickinson mdickinson deleted the fix/bad-test branch July 28, 2021 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rahulporuri rahulporuri rahulporuri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test_ignore_broken_distributions_raises_exceptions_by_default only passes by accident
2 participants
@mdickinson @rahulporuri