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

Workflows: Add scheduled e2e step and refactor ENV vars #2059

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Conversation

audiodude
Copy link
Member

@audiodude audiodude commented Jul 7, 2024

FIxes #2058

This PR refactors the ci.yml file to introduce a workflow step that only runs when the workflow is triggered by cron/scheduled. If the workflow is not triggered by cron (eg by PR), the test:e2e target is not run. So, alternately, when the workflow is triggered by a PR, the normal tests, with coverage, are run instead.

@audiodude audiodude changed the title Workflows: Refactor ENV vars and add scheduled e2e step Workflows: Add scheduled e2e step and refactor ENV vars and Jul 7, 2024
@audiodude audiodude changed the title Workflows: Add scheduled e2e step and refactor ENV vars and Workflows: Add scheduled e2e step and refactor ENV vars Jul 7, 2024
Copy link

codecov bot commented Jul 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.38%. Comparing base (dbff5d9) to head (47ab4e7).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2059      +/-   ##
==========================================
- Coverage   74.88%   74.38%   -0.51%     
==========================================
  Files          41       41              
  Lines        3146     3146              
  Branches      689      689              
==========================================
- Hits         2356     2340      -16     
- Misses        671      686      +15     
- Partials      119      120       +1     

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

@kelson42 kelson42 self-requested a review July 8, 2024 12:37
@kelson42
Copy link
Collaborator

kelson42 commented Jul 8, 2024

@audiodude I'm good with this, but I see no value to remove testing e2e on PR. I think this is an error and unit testing coverage is really not that good.

If a PR CI fails because of one of the e2e tests, then we should fix the e2e test first, I'm even ready to accept if this is in the current PR. If the PR is urgent, then we can event exceptionally shortcut the CI.

@audiodude
Copy link
Member Author

@audiodude I'm good with this, but I see no value to remove testing e2e on PR. I think this is an error and unit testing coverage is really not that good.

If a PR CI fails because of one of the e2e tests, then we should fix the e2e test first, I'm even ready to accept if this is in the current PR. If the PR is urgent, then we can event exceptionally shortcut the CI.

If you look at the code:

      - name: Running all tests (w/coverage)
        if: ${{ github.event_name != 'schedule' }}
        run: npm run codecov

The target npm run codecov runs all tests, with coverage. So the behavior on PR is unchanged.

However, the decision to only run the e2e tests on a schedule is probably flawed due to #2057. We should probably run all tests on the schedule, because many (most?) of the "unit" tests are actually e2e tests.

@audiodude audiodude force-pushed the periodic-e2e branch 2 times, most recently from d991c00 to 4acabb7 Compare July 8, 2024 19:58
@audiodude
Copy link
Member Author

audiodude commented Jul 8, 2024

I've updated the PR to run all tests on the schedule (without coverage). PTAL.

I've also renamed 'build' to 'ci-test' because it's not a build, it's running the CI tests.

Note: One thing I ran into when researching this PR is that schedule build notification emails will only get sent to me (or in the future, the last person who committed a change to ci.yml). If we want to send scheduled test failure emails more broadly, I believe we have to manually setup some kind of actions/foo-some-guy/send-email action and provide SMTP credentials and lists of email addresses in the GitHub secrets.

@kelson42 kelson42 merged commit 7f8ebd2 into main Jul 9, 2024
6 checks passed
@kelson42 kelson42 deleted the periodic-e2e branch July 9, 2024 07:31
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.

e2e tests should run periodically and generate failure emails
2 participants