Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#37] Explicitly handle transactions in the runner #38

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

swrichards
Copy link
Contributor

@swrichards swrichards commented Dec 18, 2024

Closes #37 .

@swrichards swrichards force-pushed the runner-managed-db-transactions branch from 2ce6af9 to a971f55 Compare December 18, 2024 09:54
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.37%. Comparing base (0c55577) to head (a971f55).
Report is 49 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
+ Coverage   96.89%   97.37%   +0.48%     
==========================================
  Files           8       24      +16     
  Lines         193      991     +798     
==========================================
+ Hits          187      965     +778     
- Misses          6       26      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@swrichards swrichards force-pushed the runner-managed-db-transactions branch 2 times, most recently from b2feb26 to 521414b Compare December 18, 2024 11:51
@swrichards swrichards marked this pull request as ready for review December 18, 2024 12:58
@swrichards swrichards force-pushed the runner-managed-db-transactions branch from 521414b to 82b4035 Compare December 19, 2024 17:47
@swrichards swrichards requested a review from stevenbal December 19, 2024 18:33
Copy link
Contributor

@stevenbal stevenbal left a comment

Choose a reason for hiding this comment

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

Looks good overall, just two questions

results.append(result)
yield result

if any(result.run_exception for result in results):
Copy link
Contributor

Choose a reason for hiding this comment

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

If I try this locally (for instance with Open Notificaties and have an incorrect service identifier) and add a breakpoint at line 227, the command output is:

CommandError: Error while executing step `Configuration for Notificaties API`: Service matching query does not exist. (identifier = incorrect)

And it seems that line 227 is never reached, probably because the entire thing crashes due to the CommandError which isn't caught. Is this a mistake in the implementation of the step in notifications_api_common or should that be handled here?

return {
"transaction_test_configuration_enabled": True,
"transaction_test_configuration": {"username": "alice"},
} | test_step_valid_config
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with this, isn't | an unsupported operand for two dicts?

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.

Database transactions should be explicitly handled by the runner
3 participants