When migrating tokenserver tests to use pytest instead of the builtin unittest there were a few more tests that were included (they were explicitly not included before). These tests however appear to be flaky, and are using an outdated boto client.
To ensure we don’t include unneeded code in production builds, we want to isolate any database migration-related code and ensure that unit and integration tests that reference it are also updated.
This Slack thread prompts us to do this.
Heya team - I evidently have bad luck with merges! My last one to switch over the tokenserver tests to pytest (for the test pipeline) had a failed test after merging. I didn't see the test fail in my testing but I did also notice it wasn't being explicitly included with the prior setup. I wanted to see how you all wanted to proceed? I have a few ideas I'll drop in
Ideas:
- If the tests should be running, we can mark them with a
.skip and get a bug ticket open to investigate
- OR, if they're not meant to be run, I can add in a pytest
marker to ignore them without messing up skip metrics
- OR, is there some other issue or path forward?
So, a few random thoughts:
- The Migration code is/was mostly around migrating from AWS to Spanner (IIRC). It's functionally obsolete now. There's potentially good reason to keep the code, for when we need to move the data again (this has happened twice so far, so it's not unforseeable), but it's not on the "critical path" by any stretch.
- The code uses a very old version of boto to communicate with AWS. That's clearly starting to be a problem as the warnings are specifying. Since this code is not "critical path", then skipping these tests, and including documentation about why they're being skipped would probably be a good idea.
So, perhaps for now it makes sense to keep the TestMigrationRecords* *class around - to your point it could be useful down the road - but we can just use markers to ignore it properly in pytest? Code will stick around but it won't mess up metrics as skipped tests.
Yeah, that makes sense. We'll probably want to add a quick ticket about isolating the migration code in sync so that it's done properly, but shouldn't be a huge lift.
I can include the markers to ignore them in my next pr if that works? I should have it up later today. Do I need to make the follow up ticket for isolating the migration code?
Next pr is more test pipeline stuff
Yeah, you're in the teeth of the code so it makes a bit of sense to write up the ticket. (Basically, make sure to include the reasoning so that someone can pick this up in a month or two and not be confused.)
┆Issue is synchronized with this Jira Task
When migrating tokenserver tests to use
pytestinstead of the builtinunittestthere were a few more tests that were included (they were explicitly not included before). These tests however appear to be flaky, and are using an outdatedbotoclient.To ensure we don’t include unneeded code in production builds, we want to isolate any database migration-related code and ensure that unit and integration tests that reference it are also updated.
This Slack thread prompts us to do this.
Ideas:
.skipand get a bug ticket open to investigatemarkerto ignore them without messing upskipmetricsSo, a few random thoughts:
So, perhaps for now it makes sense to keep the
TestMigrationRecords* *class around - to your point it could be useful down the road - but we can just use markers to ignore it properly in pytest? Code will stick around but it won't mess up metrics as skipped tests.Yeah, that makes sense. We'll probably want to add a quick ticket about isolating the migration code in sync so that it's done properly, but shouldn't be a huge lift.
I can include the markers to ignore them in my next pr if that works? I should have it up later today. Do I need to make the follow up ticket for isolating the migration code?
Next pr is more test pipeline stuff
Yeah, you're in the teeth of the code so it makes a bit of sense to write up the ticket. (Basically, make sure to include the reasoning so that someone can pick this up in a month or two and not be confused.)
┆Issue is synchronized with this Jira Task