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

Run vscode tests in the CI #7001

Merged
merged 25 commits into from
Jun 3, 2024
Merged

Run vscode tests in the CI #7001

merged 25 commits into from
Jun 3, 2024

Conversation

rrousselGit
Copy link
Contributor

Description

Scenarios Tested

Sample Commands

@rrousselGit rrousselGit marked this pull request as ready for review May 21, 2024 14:56
@rrousselGit rrousselGit requested a review from hlshen May 21, 2024 16:32
@hlshen hlshen requested review from joehan May 28, 2024 15:48
Copy link
Contributor

@hlshen hlshen left a comment

Choose a reason for hiding this comment

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

approved, going to let Joe take a look at this too

Copy link
Contributor

@joehan joehan left a comment

Choose a reason for hiding this comment

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

Some questions/potential minor efficiency improvements, but very glad to be adding these to CI.

with:
node-version: ${{ matrix.node-version }}
cache: npm
cache-dependency-path: npm-shrinkwrap.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be firebase-vscode/[package-lock.json instead (or in addition)? I assume we want to cache the vscode deps here.

cache-dependency-path: npm-shrinkwrap.json

- run: npm i -g npm@9.5
- run: npm ci
Copy link
Contributor

Choose a reason for hiding this comment

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

Double checking - if you remove this line, do the tests still pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt it. We have to run npm i in both the root and the vscode folder.
The npm i -g above doesn't count as it only updates the npm version and nothing else.

@joehan joehan enabled auto-merge (squash) June 3, 2024 22:30
@joehan joehan merged commit e1ff423 into master Jun 3, 2024
38 of 39 checks passed
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.

3 participants