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

chore: update test script in package.json #2699

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

Shurtu-gal
Copy link
Contributor

@Shurtu-gal Shurtu-gal commented Feb 24, 2024

Description

Changed the test script in package.json to not run the tests in the nodejs test workflow.

Copy link

netlify bot commented Feb 24, 2024

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit caaa328
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/65dde945d2118f000854d30a
😎 Deploy Preview https://deploy-preview-2699--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Feb 24, 2024

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 44
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on https://deploy-preview-2699--asyncapi-website.netlify.app/

runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
Copy link
Member

Choose a reason for hiding this comment

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

Lint checks don't differ according to the OS. So, we don't need different OS here.

Comment on lines 15 to 18
- name: Set git to use LF #to once and for all finish neverending fight between Unix and Windows
run: |
git config --global core.autocrlf false
git config --global core.eol lf
Copy link
Member

Choose a reason for hiding this comment

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

Avoid this step as we only need to run this workflow in ubuntu.

@derberg
Copy link
Member

derberg commented Feb 26, 2024

instead of adding custom workflow, I suggest we pick https://github.com/asyncapi/.github/blob/master/.github/workflows/if-nodejs-pr-testing.yml and always use the generic one. It is battle tested and has nice --if-present so commands that are not here do not run.

Yes, I know we already have workflow for cypress and we have npm run test but I think better would be to just rename test to text:components and update cypress related workflow

I always recommend to have as minimum number of custom scripts as possible

@akshatnema
Copy link
Member

akshatnema commented Feb 26, 2024

instead of adding custom workflow, I suggest we pick asyncapi/.github@master/.github/workflows/if-nodejs-pr-testing.yml and always use the generic one. It is battle tested and has nice --if-present so commands that are not here do not run.

Currently, we don't want to update in any workflows in master branch. Hence, I thought of creating this workflow only for migrate-ts branch, and will later on update the repo with node js topic, once we are ready with the tests for website. Currently, we have removed all cypress tests and will later on add them incrementally as per need.

If we add asyncapi/.github@master/.github/workflows/if-nodejs-pr-testing.ym directly to master (using nodejs topic), it will be of no use as we are not expecting any lint checks or tests to be run via this workflow.

@akshatnema akshatnema changed the base branch from master to migrate-ts February 26, 2024 17:21
@akshatnema
Copy link
Member

@Shurtu-gal Please avoid making new branch from master, if we are merging the changes in migrate-ts.

@Shurtu-gal
Copy link
Contributor Author

Shurtu-gal commented Feb 26, 2024

@derberg I had suggested the same, as we are currently trying to improve the CI/CD. If changing test command in package.json works, that's for the best, we will only need to add the node.js topic and I can close this 😄

@akshatnema akshatnema changed the base branch from migrate-ts to master February 26, 2024 17:48
@TRohit20
Copy link
Collaborator

@Shurtu-gal We can't, the PR is intended to be for migrate-ts branch but the PR is directed to master branch. Please checkout and re-route the PR target :)

@Shurtu-gal
Copy link
Contributor Author

It is meant to be in master.
Please check this thread

@akshatnema akshatnema changed the title chore: add lint workflow chore: update test script in package.json Feb 27, 2024
@akshatnema
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit 2087a0a into asyncapi:master Feb 27, 2024
19 checks passed
@Shurtu-gal Shurtu-gal deleted the eslint-workflow branch February 27, 2024 18:25
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.

5 participants