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

CI: Migrate slow test suit from Travis CI #3470

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jwagantall
Copy link

No description provided.

@@ -52,7 +53,7 @@ jobs:
packages.microsoft.com:443
- uses: actions/checkout@v4
- run: sudo apt-get update; sudo apt-get install ${{ matrix.shell }}
if: matrix.shell == 'zsh' || matrix.shell == 'ksh'
if: matrix.shell == 'zsh' || matrix.shell == 'ksh' || matrix.shell == 'bash'
Copy link
Member

@ljharb ljharb Nov 15, 2024

Choose a reason for hiding this comment

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

why does bash need to be installed separately? doesn't it come with the runner?

Copy link
Author

Choose a reason for hiding this comment

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

Line 55 gets skipped otherwise making the build fail in line 59 when we try to run which ${{ matrix.shell }}
https://github.com/nvm-sh/nvm/actions/runs/11578693670/job/32233129550

Copy link
Member

Choose a reason for hiding this comment

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

right, but that would mean that bash isn't installed by default in the ubuntu runner image?

Copy link
Author

Choose a reason for hiding this comment

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

I believe either that or maybe is not able to find it's path.. i can see if i can figure out what is happening..

Copy link
Member

Choose a reason for hiding this comment

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

hmm, maybe it's that the apt-get update needs to happen unconditionally, but bash doesn't need to be installed - either way let's land this as-is and iterate.

Copy link
Author

Choose a reason for hiding this comment

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

Seems like the slow tests are a little bit unstable, re-running a verification it actually cause the urchin ones to fail, looking into them right now if this could have been an issue with the node version..

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.

2 participants