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

RSE-1393: Add unit test github action #597

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

deb1990
Copy link

@deb1990 deb1990 commented Oct 6, 2020

Overview

This PR makes the Unit Tests Github Action to also run after a PR is merged to master. This has 2 benefits.

  1. We only run unit tests, in the PR. But it is still possible to have broken tests. If another branch made changes to a file, and it is merged to master, but another PR also might have changes to the same file. So the unit tests will pass as the current branch is not synced with master. But if we run the job again, once it is merged to master, this problem wont happen anymore.

This recently happened in compucorp/uk.co.compucorp.civiawards#222. Where we needed to fix the tests later. This happened for the exact scenario mentioned above. Because no PR had a broken test, but still the master branch was broken.

  1. In RSE-1393: Add unit tests badge #591, the logic was wrong. Even if the tests failed in a specific PR, it still used to show failed status for master branch. This PR fixes that too, as we only check for the Unit Tests run for the master branch.

@deb1990 deb1990 added the bug Something isn't working label Oct 6, 2020
@tunbola
Copy link

tunbola commented Oct 6, 2020

@deb1990
Thinking this over, I don't think it is worth adding a new github action for this, we will need to keep maintaining this alongside the other files and it basically does the same as the the workflow unit test file except that the event/trigger is different.

The checkout action we have here runs the unit test on the merged code, say for example there is a PR for a branch to master, the test is ran on the code resulting from the merge of that branch into master, see so at that point we have more or less ran tests on master and we are sure that master is not broken once when we merge that branch into master.
We don't really need this scheduled action to run every night because in essence what it is doing is running unit tests in the master branch even when the branch has not changes or no code has been merged into it seeing that the only advantage we get is a badge that tells us the build status of the master branch.

The previous implementation on #591 would have been sufficient since it did not involve duplicated code or logic and only involves adding a static asset but since it doesn't work as expected, we can do without this feature.

@deb1990 deb1990 force-pushed the RSE-1393-fix-action-badge branch 3 times, most recently from 79d6861 to 3e98b81 Compare April 23, 2021 13:25
@deb1990 deb1990 changed the title RSE-1393: Add scheduled unit test github action RSE-1393: Add unit test github action Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants