close
Skip to content

test: Re-enable scenario consistency checks after disabling them#2141

Merged
james-garner-canonical merged 2 commits intocanonical:mainfrom
james-garner-canonical:25-10+test+fix-toggling-consistency-checks
Oct 24, 2025
Merged

test: Re-enable scenario consistency checks after disabling them#2141
james-garner-canonical merged 2 commits intocanonical:mainfrom
james-garner-canonical:25-10+test+fix-toggling-consistency-checks

Conversation

@james-garner-canonical
Copy link
Copy Markdown
Contributor

@james-garner-canonical james-garner-canonical commented Oct 23, 2025

This PR corrects one of the testing tests to correctly re-enable consistency checks after turning them off. They default to on, and we run our own tests with them on.

Before this PR, they would be turned off, as we check if os.getenv(...), which will be True when the variable is set to the string 'false'.

@james-garner-canonical james-garner-canonical marked this pull request as ready for review October 23, 2025 03:37
Copy link
Copy Markdown
Collaborator

@tonyandrewmeyer tonyandrewmeyer left a comment

Choose a reason for hiding this comment

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

Interesting that nothing else had to be fixed. I guess we normally check consistency more directly, or have things consistent.

Copy link
Copy Markdown
Contributor

@dwilding dwilding left a comment

Choose a reason for hiding this comment

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

Would this be easier to follow in future if test_rubbish_event_raises became two separate tests? One test that checks for AttributeError on the non-pebble-ready events, and another that checks for AttributeError on kazoo_pebble_ready (then consistency checks could be set & unset unconditionally)

@tonyandrewmeyer
Copy link
Copy Markdown
Collaborator

Would this be easier to follow in future if test_rubbish_event_raises became two separate tests? One test that checks for AttributeError on the non-pebble-ready events, and another that checks for AttributeError on kazoo_pebble_ready (then consistency checks could be set & unset unconditionally)

It'd probably be easier to use monkeypatch.setenv then too, which seems like it would be the typical way to do this and ensure the cleanup. But good with me either way.

@james-garner-canonical
Copy link
Copy Markdown
Contributor Author

I've refactored to use two tests as you suggested, @dwilding. Would you mind having another quick look before merge?

Copy link
Copy Markdown
Contributor

@dwilding dwilding left a comment

Choose a reason for hiding this comment

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

Looks really clear to me, thanks!

@james-garner-canonical james-garner-canonical merged commit 5d4aae3 into canonical:main Oct 24, 2025
53 checks passed
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