feat: Allow setting minimum AT version from test plan report status dialog #1386
Conversation
howard-e
changed the title
| const { | ||
| isRequired, | ||
| at, | ||
| browser, | ||
| minimumAtVersion, | ||
| // minimumAtVersion, |
There was a problem hiding this comment.
You can probably just delete this?
There was a problem hiding this comment.
I have one question re: failing e2e tests on my local machine, but otherwise this feature is working for me and implementation looks ok!
One UI note that I don't think we need to fix now: I couldn't actually figure out where to pop open the dialog with the updated table because the buttons are not visually distinct or styled as buttons! I basically didn't even know those "Required Reports" buttons were even clickable until you added the steps to test! This might be something we'll want to revisit later?
|
|
||
| await page.evaluate(select => { | ||
| // Set to VoiceOver 14.0 (At.id is 5 from sample data). Default At.id was 3 (VoiceOver 11.6 (20G165)) | ||
| select.value = '5'; |
There was a problem hiding this comment.
This is the reason this test is failing for me locally. My local db is probably not in the state it needs to be in order for this test to pass, but I'm also wondering why we're not just setting the value to the id of the AT version (for example, for 14.0)... Syncing the values might make for more durable tests and maybe just makes more senes? Is there a reason we're using a separate numbering scheme for the values here?
Screenshot from a test run on my local at the moment where this test fails for me:
There was a problem hiding this comment.
Odd! Is it possible you had a conflicting db change here? There doesn't seem to be any seeders that adds VoiceOver 13.0 at least on this branch. The following screenshot of just the v11.6 and v14.0 options are what I get from a fresh hydration of the test db, with value "5" still being for 14.0:
There was a problem hiding this comment.
Ok now the test is passing locally for me now with this change! Tho I'm still curious about why we're using separate incrementing numbers for the values here as that seems a little brittle. But if it's a local test data sync issue, then maybe the bigger problem is how were setting up and tearing down test data across multiple environments.
There was a problem hiding this comment.
But if it's a local test data sync issue, then maybe the bigger problem is how were setting up and tearing down test data across multiple environments.
Yep definitely think there's more to discuss in that space
Address #1368
Testing guidelines: