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(ci/regressions): test respec-w3c differences across branches #3428

Merged
merged 17 commits into from
Apr 13, 2021

Conversation

sidvishnoi
Copy link
Member

@sidvishnoi sidvishnoi commented Mar 31, 2021

This workflow finds the impact of changes in ReSpec on real-world specs. It's run on manual requests, to avoid wasting resources. It works as follows:

  1. Build the W3C profile, and cache it (actions/upload-artifact) .
  2. For each matrix.source:
    1. Process ReSpec document using the ReSpec script in the document, and write before.html.
    2. Process ReSpec document using the ReSpec script generated in Step 1 (thanks to https://github.com/w3c/respec/pull/2957), and write after.html.
    3. Compare before.html and after.html. If there is a difference, exit with error code to draw manual attention.

A sample workflow run is available at https://github.com/w3c/respec/actions/runs/704622451 (corresponding to w3c@4aff5e7)

@sidvishnoi sidvishnoi marked this pull request as ready for review March 31, 2021 10:28
- uses: actions/checkout@v2
- uses: actions/setup-node@v1
with:
node-version: 12
Copy link
Contributor

Choose a reason for hiding this comment

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

so olde?

Copy link
Member Author

@sidvishnoi sidvishnoi Apr 6, 2021

Choose a reason for hiding this comment

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

Using v12 everywhere else and I think spec-generator is on v12 also. So, should move all of them together. Breaking change?
Node v12 goes into EOL 2022-04-30 FWIW.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should be tied to LTS instead (everywhere)... 12 seems way too limiting.

Copy link
Member Author

Choose a reason for hiding this comment

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

- name: Install git-delta # prettier git diff
run: |
set -v
wget -q -O delta.tar.gz https://github.com/dandavison/delta/releases/download/0.7.1/delta-0.7.1-x86_64-unknown-linux-gnu.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

wonder if this versioned software will become a problem to maintain? Probably not... just a thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of converting it into a separate GitHub action, which I can update as needed.
cargo install took ~4min, so had to use a pre-compiled binary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but time taken on Ci shouldn't be an issue right? That's the "cost of doing business".

Copy link
Member Author

@sidvishnoi sidvishnoi Apr 12, 2021

Choose a reason for hiding this comment

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

No issues now as I've moved this into a separate GitHub action. It takes 1s to install rather than 4min.

is 4min too much for a CI job? for git diff, yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Your meme game is strong, Padawan.

Copy link
Contributor

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Few little questions mostly... but looks good.

I'm a bit worried that we are following a moving target though... maybe we want a fake "kitchen sink" spec that tests a whole bunch of things... that might allow us to catch more errors in practice.

@sidvishnoi
Copy link
Member Author

sidvishnoi commented Apr 6, 2021

I'm a bit worried that we are following a moving target though

Do want to clarify that we'll run regression test on PR branches manually, when we feel like something in the spec might break.
Also, as tests will run on live spec using two different ReSpec versions, at almost same time, it's not much of a moving target.

that might allow us to catch more errors in practice

Might be better to test the actual practice instead of trying to copy practice in kitchen sink test?

@marcoscaceres
Copy link
Contributor

Might be better to test the actual practice instead of trying to copy practice in kitchen sink test?

Tradeoffs... happy to try this out for now and see how we go.

@sidvishnoi sidvishnoi merged commit c9e959c into develop Apr 13, 2021
@sidvishnoi sidvishnoi deleted the ci/regressions branch April 13, 2021 10:10
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