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

GH Actions: Add retry for tests on Mac OS #633

Merged
merged 10 commits into from
Mar 28, 2021
Merged

GH Actions: Add retry for tests on Mac OS #633

merged 10 commits into from
Mar 28, 2021

Conversation

seanlowjk
Copy link
Contributor

@seanlowjk seanlowjk commented Mar 9, 2021

Summary

This PR fixed the #585, the issue of Karma Tests timing out in our CI / CD pipeline.

Details

By adding the || operator, we essentially allow our pipeline to rerun the MacOSX Tests again if it failed the first time due to timeout.

Suggested Commit Message

GH Actions: Add retry for tests on Mac OS

Our Karma tests on MacOSX timeout once in a while and fail as a result.

Let's allow our pipeline to rerun our MacOSX test again in the event 
that our Karma tests on MacOSX timeout on the first try.

@codecov-io
Copy link

codecov-io commented Mar 13, 2021

Codecov Report

Merging #633 (52182dd) into master (ed6f72e) will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #633      +/-   ##
==========================================
+ Coverage   67.68%   67.72%   +0.04%     
==========================================
  Files          75       75              
  Lines        2259     2259              
  Branches      207      207              
==========================================
+ Hits         1529     1530       +1     
  Misses        686      686              
+ Partials       44       43       -1     
Impacted Files Coverage Δ
src/app/shared/issue-tables/issue-sorter.ts 52.00% <0.00%> (+4.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed6f72e...52182dd. Read the comment docs.

@seanlowjk seanlowjk requested a review from a team March 14, 2021 10:39
@seanlowjk seanlowjk marked this pull request as ready for review March 14, 2021 10:39
Copy link
Contributor

@ptvrajsk ptvrajsk left a comment

Choose a reason for hiding this comment

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

Kinda hard to test this since the tests don't always fail due to timeout, just occasionally. Seems like some users still experience the failing issue with this fix (but different OS) karma-runner/karma-chrome-launcher#154 (comment). No harm trying this fix to see if it helps our situation.

Thanks for working on this 👍

@seanlowjk
Copy link
Contributor Author

Thanks @ptvrajsk for the review ><

Copy link
Contributor

@dingyuchen dingyuchen left a comment

Choose a reason for hiding this comment

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

LGTM!

I also think there is little harm merging this, since it seems to bump up the speed a little (~0.5s).
Thanks for working on this 👍

@seanlowjk
Copy link
Contributor Author

Turns out even by removing sandboxing the MacOSX tests still fail to connect hmm..

@seanlowjk
Copy link
Contributor Author

After some discussion with @dingyuchen , another way to add on to this fix would be to rerun the command using the OR operator, || Any thoughts about this? ><

@anubh-v
Copy link
Contributor

anubh-v commented Mar 26, 2021

another way to add on to this fix would be to rerun the command using the OR operator

Thanks for investigating this issue.

Since the --no-sandbox flag is not a definitive fix, I'm in favour of leaving it out and simply adding a retry using the OR operator.

This should be sufficient, as the timeout problem does not occur very frequently (there have been 4 timeout errors, in the most recent ~120 workflow runs triggered from CATcher and CATcher-staging's master branches).

For ease of future maintenance, it will be better to not to adopt "partial fixes" like the --no-sandbox flag suggestion (especially as it is sufficient (and simpler) to add a retry).

@seanlowjk
Copy link
Contributor Author

Alright have made the changes! @anubh-v

@anubh-v anubh-v changed the title Karma Test Config: Fix Timeout for Mac OS Tests GH Actions: Add retry for tests on Mac OS Mar 27, 2021
@seanlowjk
Copy link
Contributor Author

@anubh-v I have updated the suggested commit message! Do let me know if there are any issues ><

@anubh-v anubh-v merged commit 91f9734 into CATcher-org:master Mar 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants