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.

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

functional tests: fix automatic ref updating #9690

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sshane
Copy link

@sshane sshane commented Jun 4, 2024

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Allows automatic test ref updating to function properly.

Closes #9688

@sshane sshane changed the title switch the assert order functional tests: fix automatic ref updating Jun 4, 2024
@Pierre-Sassoulas Pierre-Sassoulas added Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news πŸ”‡ This change does not require a changelog entry labels Jun 4, 2024

This comment has been minimized.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Seeing the result of the pipeline, it's likely you found an actual issue. The testutil are really poorly tested (in fact not at all directly, only indirectly from running other tests). Might be the time to actually add direct tests for them. Do you feel like doing this ? You can also upgrade the now failing tests otherwise. I'll need some time to understand what's happening to be able to review in any case.

@sshane
Copy link
Author

sshane commented Jun 4, 2024

Looks like it was asserting the previously-first assert's message?

@sshane
Copy link
Author

sshane commented Jun 4, 2024

I reverted the change to the output of LintModuleTest and made it update the output file after a failed test. I think the real solution is refactoring this test such that it isn't testing for two things/split it up. That way testutil can test for both assertion cases, but I probably don't have time for that anytime soon.

Copy link
Contributor

github-actions bot commented Jun 4, 2024

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit 63cbd3e

@Pierre-Sassoulas
Copy link
Member

I think the first commit / intention revealed an issue with the functional test : upgrading the expected output before checking the message raised or not should not make the tests fail. Maybe the expected output generation is wrong.

I think the real solution is refactoring this test such that it isn't testing for two things/split it up. That way testutil can test for both assertion cases, but I probably don't have time for that anytime soon.

Yeah, I agree, those monster classes can use composition and two sub classes anytime (and tested with more ease as a result). Not sure this is strictly necessary to fix the issue you unearthed though.

I probably don't have time for that anytime soon.

No worries, thank you for bringing the issue to our attention πŸ˜„

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug πŸͺ² Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news πŸ”‡ This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't update functional test refs
2 participants