Skip to content

Adding conditional github actions job execution #1417

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

Open
wants to merge 25 commits into
base: develop
Choose a base branch
from

Conversation

asuresh-code
Copy link
Contributor

@asuresh-code asuresh-code commented May 8, 2025

Description

This is an experimental PR to configure the github actions jobs to not all execute on every commit.

Summary:
Advice is to not implement any of the options; Options 1 and 2 provide a small efficiency improvement (1-2%), and Option 3 is not viable (explained in below sections). This PR, which is investigating using test selection in the jobs themselves is much more promising. 1 possible area of further investigation is configuring the dependancy bot PRs, since they make up a sizeable chunk of jobs ran.

Configuration Options:
Option 1. Do not run any build jobs if only certain files are changed (mainly documentation i.e, README, License, etc)

Option 2. Do not run individual test jobs if only unrelated test files are changed in this PR (i.e unit tests are skipped if only cypress files are changed in the whole PR history (since branching from develop)) (Done for unit tests)

Option 3. Do not run individual test jobs if only unrelated test files are changed in this commit (i.e unit tests are skipped if only cypress files are changed in the current commit, any component file changed triggers all tests).

N.B Option 3 does not seem plausible anymore. If implemented like this, it would ignore changes made to files in develop when running tests. For instance if in a commit, no unit test files are changed then unit tests would not be ran, even if they were changed in develop after branching off of it. This poses a problem for merging PRs; 1 solution would be to force a manual workflow everytime before merging, but that seems cumbersome, and would offset some efficiency gains.

Analysis:
A random sample of 50 human commits was taken from this repository from the last 6 months, and each was evaluated to see if the above proposed configurations would have any effect. Results are as follows:

  • Option 1: 0% Reduction in number of test jobs run
  • Option 2: 1% Reduction in number of test jobs run
  • Option 3: 13% Reduction in number of test jobs run

N.B. Test Job refers to a job in the context of Github actions workflows, i.e every workflow has 3 test jobs (unit, e2e w/ api, e2e mock)

Testing instructions

Branch off of this branch, open a new pull request, and from there you can checkout commits for the different option configurations and push file changes to see which jobs get run.

  • Review code
  • Check Actions build
  • Review changes to test coverage

Agile board tracking

Closes #1458

Copy link

codecov bot commented May 8, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
776 1 775 1
View the top 1 failed test(s) by shortest run time
src/admin/adminCardView.component.test.tsx > AdminCardView > renders admin card view correctly
Stack Traces | 1.12s run time
TestingLibraryElementError: Unable to find an element with the text: u. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

Ignored nodes: comments, script, style
<body>
  <div>
    <div
      class="MuiGrid-root MuiGrid-container css-11lq3yg-MuiGrid-root"
    >
      <div
        class="MuiGrid-root MuiGrid-container css-k6wrnl-MuiGrid-root"
      >
        <div
          class="MuiGrid-root MuiGrid-container MuiGrid-item MuiGrid-grid-xs-12 css-spompt-MuiGrid-root"
        >
          <div
            class="MuiGrid-root MuiGrid-item MuiGrid-grid-xs-12 MuiGrid-grid-sm-6 css-1b3l6lk-MuiGrid-root"
          >
            <a
              class="MuiButtonBase-root MuiButton-root MuiButton-text MuiButton-textPrimary MuiButton-sizeMedium MuiButton-textSizeMedium MuiButton-colorPrimary MuiButton-fullWidth MuiButton-root MuiButton-text MuiButton-textPrimary MuiButton-sizeMedium MuiButton-textSizeMedium MuiButton-colorPrimary MuiButton-fullWidth css-q760su-MuiButtonBase-root-MuiButton-root"
              href="/units"
              tabindex="0"
            >
              <div
                class="MuiPaper-root MuiPaper-elevation MuiPaper-rounded MuiPaper-elevation1 MuiCard-root css-siqlql-MuiPaper-root-MuiCard-root"
              >
                <div
                  class="MuiCardContent-root css-lo1d59-MuiCardContent-root"
                >
                  <div
                    class="MuiGrid-root css-vj1n65-MuiGrid-root"
                  >
                    <div
                      class="MuiGrid-root css-13qkvwv-MuiGrid-root"
                    >
                      <p
                        class="MuiTypography-root MuiTypography-body1 css-ahj2mt-MuiTypography-root"
                      >
                        Units
                      </p>
                    </div>
                  </div>
                </div>
              </div>
              <span
                class="MuiTouchRipple-root css-8je8zh-MuiTouchRipple-root"
              />
            </a>
          </div>
          <div
            class="MuiGrid-root MuiGrid-item MuiGrid-grid-xs-12 MuiGrid-grid-sm-6 css-1b3l6lk-MuiGrid-root"
          >
            <a
              class="MuiButtonBase-root MuiButton-root MuiButton-text MuiButton-textPrimary MuiButton-sizeMedium MuiButton-textSizeMedium MuiButton-colorPrimary MuiButton-fullWidth MuiButton-root MuiButton-text MuiButton-textPrimary MuiButton-sizeMedium MuiButton-textSizeMedium MuiButton-colorPrimary MuiButton-fullWidth css-q760su-MuiButtonBase-root-MuiButton-root"
              href="/usage-statuses"
              tabindex="0"
            >
              <div
                class="MuiPaper-root MuiPaper-elevation MuiPaper-rounded MuiPaper-elevation1 MuiCard-root css-siqlql-MuiPaper-root-MuiCard-root"
              >
                <div
                  class="MuiCardContent-root css-lo1d59-MuiCardContent-root"
                >
                  <div
                    class="MuiGrid-root css-vj1n65-MuiGrid-root"
                  >
                    <div
                      class="MuiGrid-root css-13qkvwv-MuiGrid-root"
                    >
                      <p
                        class="MuiTypography-root MuiTypography-body1 css-ahj2mt-MuiTypography-root"
                      >
                        Usage Statuses
                      </p>
                    </div>
                  </div>
                </div>
              </div>
              <span
                class="MuiTouchRipple-root css-8je8zh-MuiTouchRipple-root"
              />
            </a>
          </div>
        </div>
      </div>
    </div>
  </div>
</body>

Ignored nodes: comments, script, style
<html>
  <head />
  <body>
    <div>
      <div
        class="MuiGrid-root MuiGrid-container css-11lq3yg-MuiGrid-root"
      >
        <div
          class="MuiGrid-root MuiGrid-container css-k6wrnl-MuiGrid-root"
        >
          <div
            class="MuiGrid-root MuiGrid-container MuiGrid-item MuiGrid-grid-xs-12 css-spompt-MuiGrid-root"
          >
            <div
              class="MuiGrid-root MuiGrid-item MuiGrid-grid-xs-12 MuiGrid-grid-sm-6 css-1b3l6lk-MuiGrid-root"
            >
              <a
                class="MuiButtonBase-root MuiButton-root MuiButton-text MuiButton-textPrimary MuiButton-sizeMedium MuiButton-textSizeMedium MuiButton-colorPrimary MuiButton-fullWidth MuiButton-root MuiButton-text MuiButton-textPrimary MuiButton-sizeMedium MuiButton-textSizeMedium MuiButton-colorPrimary MuiButton-fullWidth css-q760su-MuiButtonBase-root-MuiButton-root"
                href="/units"
                tabindex="0"
              >
                <div
                  class="MuiPaper-root MuiPaper-elevation MuiPaper-rounded MuiPaper-elevation1 MuiCard-root css-siqlql-MuiPaper-root-MuiCard-root"
                >
                  <div
                    class="MuiCardContent-root css-lo1d59-MuiCardContent-root"
                  >
                    <div
                      class="MuiGrid-root css-vj1n65-MuiGrid-root"
                    >
                      <div
                        class="MuiGrid-root css-13qkvwv-MuiGrid-root"
                      >
                        <p
                          class="MuiTypography-root MuiTypography-body1 css-ahj2mt-MuiTypography-root"
                        >
                          Units
                        </p>
                      </div>
                    </div>
                  </div>
                </div>
                <span
                  class="MuiTouchRipple-root css-8je8zh-MuiTouchRipple-root"
                />
              </a>
            </div>
            <div
              class="MuiGrid-root MuiGrid-item MuiGrid-grid-xs-12 MuiGrid-grid-sm-6 css-1b3l6lk-MuiGrid-root"
            >
              <a
                class="MuiButtonBase-root MuiButton-root MuiButton-text MuiButton-textPrimary MuiButton-sizeMedium MuiButton-textSizeMedium MuiButton-colorPrimary MuiButton-fullWidth MuiButton-root MuiButton-text MuiButton-textPrimary MuiButton-sizeMedium MuiButton-textSizeMedium MuiButton-colorPrimary MuiButton-fullWidth css-q760su-MuiButtonBase-root-MuiButton-root"
                href="/usage-statuses"
                tabindex="0"
              >
                <div
                  class="MuiPaper-root MuiPaper-elevation MuiPaper-rounded MuiPaper-elevation1 MuiCard-root css-siqlql-MuiPaper-root-MuiCard-root"
                >
                  <div
                    class="MuiCardContent-root css-lo1d59-MuiCardContent-root"
                  >
                    <div
                      class="MuiGrid-root css-vj1n65-MuiGrid-root"
                    >
                      <div
                        class="MuiGrid-root css-13qkvwv-MuiGrid-root"
                      >
                        <p
                          class="MuiTypography-root MuiTypography-body1 css-ahj2mt-MuiTypography-root"
                        >
                          Usage Statuses
                        </p>
                      </div>
                    </div>
                  </div>
                </div>
                <span
                  class="MuiTouchRipple-root css-8je8zh-MuiTouchRipple-root"
                />
              </a>
            </div>
          </div>
        </div>
      </div>
    </div>
  </body>
</html>...
 ❯ waitForWrapper node_modules/@.../dom/dist/wait-for.js:163:27
 ❯ src/admin/adminCardView.component.test.tsx:12:11

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@asuresh-code asuresh-code force-pushed the github-actions-smart-configuration branch from 9e13c15 to 9856db7 Compare May 8, 2025 08:25
@asuresh-code asuresh-code force-pushed the github-actions-smart-configuration branch from 9856db7 to f33fbc4 Compare May 8, 2025 08:41
@asuresh-code asuresh-code force-pushed the github-actions-smart-configuration branch from 4fa1d02 to 7d86fce Compare May 8, 2025 10:03
@asuresh-code asuresh-code force-pushed the github-actions-smart-configuration branch from 58dd33b to 220739b Compare May 15, 2025 10:41
@asuresh-code asuresh-code force-pushed the github-actions-smart-configuration branch from 220739b to 7ea05ed Compare May 15, 2025 10:43
…t branch creation, instead of base branch now
@asuresh-code asuresh-code force-pushed the github-actions-smart-configuration branch from 0bf931a to 6b023ad Compare May 15, 2025 11:00
@asuresh-code asuresh-code force-pushed the github-actions-smart-configuration branch from a7311af to ecd1227 Compare May 15, 2025 13:04
@asuresh-code asuresh-code marked this pull request as draft May 19, 2025 07:05
@asuresh-code asuresh-code self-assigned this May 21, 2025
LICENSE Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are some unintended changes here, in the README and in some of the tests below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out; I've undone the extra changes in random files (they were for testing the job execution), and I've put 3 commits for the different setup configurations.

@asuresh-code asuresh-code requested a review from joelvdavies June 4, 2025 13:25
@joelvdavies joelvdavies mentioned this pull request Jun 5, 2025
3 tasks
Copy link
Contributor

@joelvdavies joelvdavies left a comment

Choose a reason for hiding this comment

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

Currently testing option 3, just noticed that a change to a unit test file itself doesn't currently get picked up as a change.

Also tested merging in a conflicting change - the merge commit didn't seem to trigger (I think that was my fault though) so I triggered it manually and the tests did run on the merge changes as expected.

As discussed I think the biggest issue with option 3 is the fact that if I change a file, break the tests then in another commit make a change elsewhere e.g. the readme, the CI will pass on the whole PR even though there is still a broken test somewhere. Like you said one way around it would be to use artifacts to carry over the requests but I wonder if that might make it too complicated. Overall I currently think option 2 would be the best one for now.

@asuresh-code
Copy link
Contributor Author

Overall I currently think option 2 would be the best one for now.

Agreed, Option 3 has some extra caveats required as well to make it work; for instance, you need to run it one more time before merging, including all files changed in the PR, (not just the last commit), to pick up any relevant changes from develop, which adds more complexity/hassle.

Option 2, albeit a small improvement, doesn't have any drawback really, other than maintenance of the extra LOC.

@joelvdavies joelvdavies force-pushed the github-actions-smart-configuration branch from 4f444fa to 79ed40a Compare July 24, 2025 10:11
@joelvdavies joelvdavies marked this pull request as ready for review July 24, 2025 10:13
@joelvdavies joelvdavies self-requested a review July 24, 2025 10:13
@joelvdavies
Copy link
Contributor

joelvdavies commented Jul 24, 2025

The status checks above are stuck: Manually triggering seems to work e.g. https://github.com/ral-facilities/inventory-management-system/actions/runs/16494115304/job/46635643993.

After investigating it seems its because of the new detect_changes step, the branch protections need to be modified to include it but its not on develop so doing so will cause the same issue for all other PRs.

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.

Investigate CI Build job selection
2 participants