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

Fix possible MusicController nullref #2618

Merged
merged 3 commits into from May 25, 2018

Conversation

2 participants
@smoogipoo
Contributor

smoogipoo commented May 23, 2018

Because Beatmap bindable can be set by testcases, this is a possibility.

TestCases really need to not change the beatmap bindable but that's a pretty big change.

@smoogipoo smoogipoo added the bug label May 23, 2018

@smoogipoo smoogipoo added this to the May 2018 milestone May 23, 2018

@peppy

This comment has been minimized.

Member

peppy commented May 25, 2018

If that's the case, shouldn't the MusicController test case ensure the beatmap is set as required?

The beatmap it's binding to is a NonNullable, so I don't think this is the correct way to fix this.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented May 25, 2018

It's not that the beatmap is null. It's that the beatmap set is null, which happens from testcases that set their own beatmaps.

@peppy

This comment has been minimized.

Member

peppy commented May 25, 2018

The first null coalesce can be removed then.

I'd rather fix the testcase instead of this class. BeatmapSet is supposed to be non-null too. As mentioned though, MusicController should be setting a valid beatmap before it loads the component.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented May 25, 2018

It's not any one testcase. At the moment it's literally every testcase that modifies the beatmap, including but not limited to AllPlayers.

Can remove the first null coalesce, sure. But I don't think beatmaps are supposed to always have a beatmap set. The instantiation of a beatmap would be absolutely insane then.

@peppy

This comment has been minimized.

Member

peppy commented May 25, 2018

each testcase that uses a beatmap should set a relevant beatmap on startup, even so.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented May 25, 2018

You're missing the point of this change.

@peppy

This comment has been minimized.

Member

peppy commented May 25, 2018

This change is still valid. I'm saying on top of this test cases should be specifying valid defaults so order of execution is not an issue.

peppy added some commits May 25, 2018

@peppy

peppy approved these changes May 25, 2018

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented May 25, 2018

Testcases should set defaults that are applicable only to them and those defaults should not be seen by nor have an affect on other testcases.

The beatmap is isolated to testcases in my GameBeatmap PR, which follows this principle.

@peppy peppy merged commit 1c947fd into ppy:master May 25, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@smoogipoo smoogipoo deleted the smoogipoo:musiccontroller-nullref branch Jun 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment