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: restore yarn test, excluding e2e; restore node LTS #78

Merged
merged 3 commits into from
Aug 6, 2024
Merged

Conversation

dckc
Copy link
Member

@dckc dckc commented Apr 18, 2024

fixes #77

@dckc dckc requested a review from 0xpatrickdev April 18, 2024 14:43
@@ -35,6 +35,8 @@ jobs:
run: yarn start:docker
- name: yarn build
run: yarn build
- name: yarn test
run: yarn test --exclude "**/e2e/**"
- name: yarn start:contract
Copy link
Member

Choose a reason for hiding this comment

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

Maybe better to stick this in the unit job? L23-L24

Copy link
Member Author

@dckc dckc Apr 18, 2024

Choose a reason for hiding this comment

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

oops! yes, that's where I meant to put it...

p.s. I presume you don't mind a force push

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right either:

- name: Use Node.js 18.8.x
uses: actions/setup-node@v3
with:
# use node 18.8.x until Agoric/agoric-sdk#8636
node-version: "18.8.x"
, and Agoric/agoric-sdk#8636 is now closed. Maybe just 18.x, or ['18.x', '20.x'] since it's a quick job?

Copy link
Member Author

@dckc dckc Apr 18, 2024

Choose a reason for hiding this comment

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

@dckc dckc force-pushed the dc-yarn-test branch 2 times, most recently from 56c7a48 to aebfd29 Compare April 18, 2024 15:12
@dckc dckc enabled auto-merge (rebase) April 18, 2024 15:13
@dckc dckc disabled auto-merge April 18, 2024 15:15
@dckc
Copy link
Member Author

dckc commented Apr 18, 2024

(disabling auto-merge until Agoric/agoric-sdk#9250 lands)

@dckc dckc changed the title ci: restore yarn test, excluding e2e ci: restore yarn test, excluding e2e; restore node LTS Apr 18, 2024
@dckc
Copy link
Member Author

dckc commented May 10, 2024

odd... ci checks aren't running. I don't see why not.
clues, @rabi-siddique? @0xpatrickdev ?

@0xpatrickdev
Copy link
Member

0xpatrickdev commented May 10, 2024

odd... ci checks aren't running. I don't see why not. clues, @rabi-siddique? @0xpatrickdev ?

I think we might need to do something like:

    runs-on: ubuntu-latest
    strategy:
      matrix:
        node-version: ['18.x', '20.x']
    steps:
      - name: Checkout Repo
        uses: actions/checkout@v3
      - name: Use Node.js LTS versions
        uses: actions/setup-node@v3
        with:
          node-version: ${{ matrix.node-version }}

See https://github.com/Agoric/agoric-sdk/blob/c11fecbb0105bde2631adc14e106daf19c3f1201/.github/workflow-templates/test-dapp.yml#L9-L21

@amessbee
Copy link
Contributor

amessbee commented Aug 6, 2024

@dckc can we merge this now?

@dckc
Copy link
Member Author

dckc commented Aug 6, 2024

@dckc can we merge this now?

It doesn't seem to work. I see Some checks haven’t completed yet and I don't see how to get them to complete.

If what you want is the change to node versions, you could make a PR with just that commit.

uses: actions/setup-node@v3
with:
# use node 18.8.x until Agoric/agoric-sdk#8636
node-version: "18.8.x"
node-version: ['18.x', '20.x']
Copy link
Contributor

Choose a reason for hiding this comment

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

@dckc Have you tried changing this to just a string instead of an array?
@frazarshad suggests:
"i think instead we can use matrix testing if we want to test on multiple node versions"

Copy link
Member Author

Choose a reason for hiding this comment

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

This was an attempt to do matrix testing. We should be testing on 18 and 20. Ah... it look like patrick's comment is saying roughly the same thing. I can try that...

@frazarshad
Copy link

@dckc @amessbee i think the CI checks arent passing because we've set unit as a required check for all PRs. but essentially we've replaced unit with unit (18) and unit (20) so thats why it cant pass that check.
can we try updating the required checks for this repo?

@dckc dckc merged commit 4debeb9 into main Aug 6, 2024
3 checks passed
@dckc dckc deleted the dc-yarn-test branch August 6, 2024 15:53
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.

ci does not run yarn test
4 participants