Conversation
gartnera
added
no-changelog
📝 WalkthroughWalkthroughThe pull request introduces modifications to the GitHub Actions workflows in Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
gartnera
force-pushed
the
run-e2e-twice
branch
from
0ede48a to
d34a585
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3262 +/- ##
===========================================
+ Coverage 61.74% 61.78% +0.03%
===========================================
Files 431 431
Lines 30772 30786 +14
===========================================
+ Hits 19001 19021 +20
+ Misses 10914 10909 -5
+ Partials 857 856 -1 |
gartnera
force-pushed
the
run-e2e-twice
branch
from
d34a585 to
a3bf34a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/reusable-e2e.yml (1)
87-90: Fix potential word splitting in container ID assignmentThe current shell script might be susceptible to word splitting issues.
Apply this diff to make the script more robust:
run: | - container_id=$(docker ps --filter "ancestor=orchestrator:latest" --format "{{.ID}}") + container_id="$(docker ps --filter "ancestor=orchestrator:latest" --format '{{.ID}}')" docker logs --since -10s -f "${container_id}" & exit $(docker wait "${container_id}")🧰 Tools
🪛 actionlint (1.7.4)
87-87: shellcheck reported issue in this script: SC2046:warning:3:6: Quote this to prevent word splitting
(shellcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/e2e.yml(4 hunks).github/workflows/reusable-e2e.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/reusable-e2e.yml
87-87: shellcheck reported issue in this script: SC2046:warning:3:6: Quote this to prevent word splitting
(shellcheck)
🔇 Additional comments (1)
.github/workflows/e2e.yml (1)
126-126: LGTM! Implementation aligns with PR objectives
The changes effectively implement the requirement to run e2e tests twice:
- Configurable via PR label "TWICE_TESTS"
- Automatically enabled for merge queue
- Selectively applied to default e2e tests only
Also applies to: 160-160, 164-164, 230-230, 277-277
skosito
left a comment
skosito
left a comment
There was a problem hiding this comment.
lgtm but would be nice to add more context in PR description
lumtis
left a comment
lumtis
left a comment
There was a problem hiding this comment.
Adds ~8 minutes to mergequeue times.
Can't we run these concurrently instead of sequentially?
No since the point of the second run is to ensure that the tests work given the state leftover from the first run.
Yeah we could do that but it would take a bit longer (looks like +5 minutes) |
lumtis
left a comment
lumtis
left a comment
There was a problem hiding this comment.
No since the point of the second run is to ensure that the tests work given the state leftover from the second run.
I see. I'm in if it can catch edge cases
Just curious, is 5 minutes a significant delay here? It does provide the value of testing the TSS migration as well. And reuse exiting code |
Yeah maybe an extra 5 minutes isn't very significant. And I think the tss migrations tests may be stable enough to run in the mergequeue now too. |
|
@gartnera why was this one put in draft? |
I was going to try to get the tss migration tests running instead like Tanmay suggested. But I haven't gotten around to seeing if it will actually work yet. |
4 participants
Add option to run default e2e tests twice in CI. Always run twice in merge queue.
We want to ensure that the default e2e tests are never dependent on the system state.
Adds ~8 minutes to mergequeue times.
Summary by CodeRabbit
New Features
TWICE_TESTSto conditionally run tests multiple times based on pull request labels.run-twiceto the reusable E2E testing workflow, allowing for a second execution of tests.run-twiceinput.Bug Fixes