Skip to content

Conversation

@nshirley
Copy link
Collaborator

@nshirley nshirley commented Mar 14, 2025

Description

Describe these changes.

NOTE: We can only accept PRS with all commits signed. PRs that contain any unsigned commits will not be accepted and the PR must be resubmitted. If this is something you cannot provide, please disclose and a team member may duplicate the PR as signed for you (depending on availablity and priority. Thank you for your understanding and cooperation.)

The original ticket was scoped to move all python tests (tokenserver, mysqle2e and spannere2e) to utilize pytest, but for the sake of speed the latter two will be tackled in another ticket.

Interestingly, by using pytest it can discover the existing unittest tests and run them without extra work at this time, and in doing so, it's also running more tests that were not being run before! Here are the tests that additionally run:

TestProcessAccountEventsForceSpanner::test_delete_user_force_spanner
TestMigrationRecords::test_purging_no_override
TestMigrationRecords::test_purging_override_null_keys_changed_at
TestMigrationRecords::test_purging_override_with_migrated
TestMigrationRecords::test_purging_override_with_migrated_password
TestMigrationRecords::test_purging_replaced_at

Should we keep these tests in? Do they need to run?

Testing

How should reviewers test?

Issue(s)

Closes SYNC-4516.

@nshirley nshirley changed the title Sync 4612 migrate to pytest chore: migrate tokenserver tests to pytest with junit output Mar 14, 2025
@nshirley nshirley marked this pull request as ready for review March 14, 2025 23:59
@nshirley nshirley requested review from a team and Trinaa March 14, 2025 23:59
Copy link

@Trinaa Trinaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢. Don't forget to squash before merging.

What is the ticket number for the remaining work for e2e tests? I don't see it in the epic atm.

cargo llvm-cov report --summary-only --json --output-path ${UNIT_COVERAGE_JSON}
cargo llvm-cov report --summary-only --json --output-path ${UNIT_COVERAGE_JSON}

.ONESHELL:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: more of a note to @Trinaa and @jrconlin as we have this in autopush too -- it seems .ONESHELL is a Makefile wide special target that only needs to be set once, "If the .ONESHELL special target appears anywhere in the makefile then all recipe lines for each target.."

@pjenvey
Copy link
Member

pjenvey commented Mar 17, 2025

@nshirley and don't forget to add a "chore:" prefix to the commit when you squash

@nshirley nshirley merged commit 15840c5 into master Mar 18, 2025
9 checks passed
@nshirley nshirley deleted the SYNC-4612-migrate-to-pytest branch March 18, 2025 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants