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

Ensure all mods are incompatible in both ways #19203

Merged
merged 8 commits into from
Aug 26, 2022

Conversation

tsunyoku
Copy link
Member

After #19200 and #19201, it seemed appropriate to add an extra test that all mods are two-way incompatible.

@tsunyoku
Copy link
Member Author

tsunyoku commented Jul 17, 2022

Tests seem to be failing as it fails to find the instance of the mod it's comparing to (in some cases). The line which is causing it: https://github.com/ppy/osu/pull/19203/files#diff-38c0d7d7fdbeef4006389cbdab55fdb19f0c26c9152d500287ea7e8d91ed7765R39

Kinda unsure why, maybe someone else has an idea?

@ggliv
Copy link
Contributor

ggliv commented Jul 18, 2022

Mods can declare incompatibility with other mods that may not necessarily be included in the same ruleset. For example, CatchModHalfTime inherits incompatibility with ModAdaptiveSpeed from ModRateAdjust despite the catch ruleset not having an implementation of ModAdaptiveSpeed.

I'd suggest that you run the test locally with a debugger attached before pushing more commits to avoid unnecessary CI congestion.

@frenzibyte frenzibyte changed the title rename NoConflictingModAcronyms test to ModValidity, add test for two-way mod incompatibilities Ensure all mods are incompatible in both ways Jul 18, 2022
@peppy
Copy link
Member

peppy commented Jul 18, 2022

Kinda unsure why, maybe someone else has an idea?

Are you unable to debug this locally? It should be immediately apparent by checking what the fail cases are (this is a 100% failure).

@peppy peppy self-requested a review August 26, 2022 10:48
@peppy
Copy link
Member

peppy commented Aug 26, 2022

As this seemed abandoned I fixed the remaining issue with the test. Looks to work as expected.

@peppy peppy enabled auto-merge August 26, 2022 11:00
@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Aug 26, 2022
@smoogipoo smoogipoo disabled auto-merge August 26, 2022 14:01
@smoogipoo smoogipoo merged commit 8b871da into ppy:master Aug 26, 2022
@tsunyoku tsunyoku deleted the mod-validation-tests branch November 12, 2022 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:mods next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/M type:testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants