Revert "Revert "Age Filter - Signed In"" #25724
Conversation
|
@islemaster Any ideas what might have caused the test failures? |
|
Here's the most recent failure I see for this test.
I noticed this is different from other failures on this test, but sometimes cucumber is inconsistent with output so it might be fortunate to get this error output. Another test failure with truncated output ends right at this step, so there's a chance they're the same failure mode:
So for now let's assume the step we need to fix is
Anyway, I pulled up the screencast, and when this test fails you can't actually see the dropdown options, but you can see what the first item is; my first thought was this looks like the same default song I've seen on staging/test/levelbuilder, so I wonder if a recent content change is interfering with your test - I think we always have more than one song in the song selector now.
Just to confirm, I found the exact step in SauceLabs that's checking the number of elements in the dropdown, and sure enough the selector is finding lots of elements:
So I wonder if we need to change the approach in this test. Maybe we could check the dropdown for the presence/absence of a particular known "pg13" song, rather than the count of songs since that might continue to change. One gotcha here is that this failure probably won't repro on local/circle because we're using a different song manifest in that situation. It might be worth thinking about ways for the test to depends on something available in both environments. The other thing I haven't figured out is why this is flaky; I'd expect it to fail all the time if there are just more songs in the manifest now. Later tests are failing on:
...which also doesn't seem like something that would be caused by this PR, but who knows? I'd go with the policy of fixing the first error first and seeing if anything downstream gets cleaned up. |
|
Thanks for the advice! I also discovered another issue with this test, but forgot to update my request last night. |
whether to turn on the filter for signed in students.
|
@islemaster PTAL - thanks! |
|
Have to run but can take a look in an hour or so. |
| @@ -45,7 +48,7 @@ | |||
|
|
|||
| #gdpr-dialog | |||
|
|
|||
| %script{src: minifiable_asset_path('js/code-studio.js'), data: {gdpr: gdpr_data.to_json}} | |||
| %script{src: minifiable_asset_path('js/code-studio.js'), data: {gdpr: gdpr_data.to_json, user: user_type.to_json}} | |||
There was a problem hiding this comment.
Why user rather than user_type as the key here?
| }; | ||
|
|
||
| state = { | ||
| songsData: [] | ||
| songsData: [], | ||
| filterOff: this.props.userType !== 'student_y' |
There was a problem hiding this comment.
This logic turns the filter off when userType is unknown. It seems like we should write this to err on the 'safe' side so we only turn the filter off if the userType is one of a known set of 'older user' types.
| }; | ||
|
|
||
| state = { | ||
| songsData: [] | ||
| songsData: [], | ||
| filterOff: this.props.userType !== 'student_y' |
There was a problem hiding this comment.
Nit: You only set filterOff when constructing the component, so it could be a prop rather than React state (you'd move this expression into your connect() method).
Reverts #25718
Reverted to isolate test failures. DTT passed with retries after these edits were reverted. Not sure what about these changes might cause errors (tests would pass the first scenario, then stop testing and show up as failed).