Skip to content

Conversation

sckott
Copy link
Collaborator

@sckott sckott commented May 2, 2025

fix #168

definitely squash and merge - lots of iterating to fix actions commits

Here's a run of this new workflow: https://github.com/FredHutch/wdl-unit-tests/actions/runs/14801625090/job/41732524382

Here's the generated PR from the above workflow run: #169

Changes here:

  • now using pytest plugin pytest-json-report
  • added fixture pytest_json_modifyreport that when the --json-report flag is used with pytest a report of test results is written to a json file. this fixture is not used when the --json-report flag is absent via this line @pytest.hookimpl(optionalhook=True)
  • reformatted some make commands
  • new make command test_api_rewrite_json_report to do cassette rewrites AND generate a json report
  • modified the rewrite cassettes github action yaml to
    • generate the json report when testing
    • always succeed via exit 0 (otherwise any failures would not let us proceed)
    • open a PR with only the paths to cassettes and mock json files for tests that passed (failed test files are ignored)
    • then either Sean or I would merge the PR
    • the idea is for likely me to investigate any failures and fix those, then we can run rewrite cassettes again and merge that

¿Bueno?

@sckott sckott added the infrastructure Infrastructure fix to execute WDL GitHub Actions label May 2, 2025
@sckott sckott added this to the v0.2 test infrastructure milestone May 2, 2025
@sckott sckott requested review from seankross and tefirman May 2, 2025 19:06
@sckott sckott marked this pull request as ready for review May 6, 2025 19:38
@tefirman
Copy link
Collaborator

tefirman commented May 7, 2025

@sckott -- I like this addition, should make debugging much easier, but I'm a little confused where to look for the list of passed/failed test. I went to the GitHub Action linked above and the "List all cassettes passed" function just says "Array", same with failed cassettes. Am I looking in the wrong place?

image

@sckott
Copy link
Collaborator Author

sckott commented May 8, 2025

@tefirman yeah I need to fix that. Its printing the class of the array of tests that failed or succeeded rather than the actual list

@sckott
Copy link
Collaborator Author

sckott commented May 8, 2025

Waiting on fix to the test user from Dan

@sckott sckott added blocked We're blocked! and removed blocked We're blocked! labels May 8, 2025
@sckott
Copy link
Collaborator Author

sckott commented May 9, 2025

@tefirman okay, we're unblocked now by fix to test user, but the failed and succeeded tests are still not showing up correctly ...

@sckott
Copy link
Collaborator Author

sckott commented May 12, 2025

Okay, @tefirman and @seankross can you take another look please - see last run https://github.com/FredHutch/wdl-unit-tests/actions/runs/14979902855

We're now doing the following approach:

  • run test with make test_api_rewrite_json_report which runs tests and writes a report to a json file
  • undo any changes to cassettes and mock json files associated with FAILED tests
  • add only paths to the PR within the dirs: tests/cromwellapi/cassettes/* and tests/cromwellapi/mocked_submissions/*

With the current state of this action I haven't seen how it behaves when tests fail yet, but I'm pretty sure it will work well.

Copy link
Collaborator

@seankross seankross left a comment

Choose a reason for hiding this comment

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

Thank you so much Scott, it looks clean but I can smell how this went through several painful iterations. I think this is right on the money though.

- name: Run tests and process changes
id: process-changes
run: |
make test_api_rewrite_json_report || true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love the elegance of make [command] || true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, there's many ways to achieve this but this one seems good

@sckott
Copy link
Collaborator Author

sckott commented May 12, 2025

thanks @seankross

Copy link
Collaborator

@tefirman tefirman left a comment

Choose a reason for hiding this comment

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

Looks awesome, really like the clear call-out of failing tests. Thanks for putting this together, @sckott !

@sckott
Copy link
Collaborator Author

sckott commented May 12, 2025

thanks. @tefirman

@sckott sckott merged commit 81aae11 into main May 12, 2025
1 check passed
@sckott sckott deleted the rewrite-cassettes-retry branch May 19, 2025 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Infrastructure fix to execute WDL GitHub Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test rewrites fixes
3 participants