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: replace scripts with xtask pattern #1097

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

simonbuchan
Copy link
Contributor

Description:

Migrates scripts to use xtask pattern.

Related issue (if exists):

See discussion in #1096

@changeset-bot
Copy link

changeset-bot bot commented Oct 5, 2023

⚠️ No Changeset found

Latest commit: b348e8d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@simonbuchan simonbuchan changed the title Draft: chore: replace scripts with xtask pattern WIP: chore: replace scripts with xtask pattern Oct 5, 2023
@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Thank you for the PR!
Commit: b348e8d

Files to check

These are files which is affected by the current PR, but not reflected. If there's no file below this message, please ignore this message.

You can run cargo xtask auto-unignore for typescript files, and cargo xtask test-checker for *.stats.rust-debug files.

crates/stc_ts_type_checker/tests/conformance.pass.txt

@kdy1 kdy1 marked this pull request as draft October 5, 2023 08:10
@simonbuchan
Copy link
Contributor Author

Not dead yet! I got distracted trying to figure out why mimalloc-rust (used by swc_node_base on non-linux platforms) was being all crashy, but that should be a separate PR if I do figure it out.

This is pretty close for what's implemented: the enabled tests for stc_ts_{file_analyzer,type_checker}.

The main thing left I know about is that cargo xtask auto-unignore is generating a bunch of .*.tsc-errors.json files outside of tests/tsc, so they aren't getting cleaned up. I have no idea what I'm doing differently to the shell script here, unfortunately.

@kdy1
Copy link
Member

kdy1 commented Oct 10, 2023

I can take a look when I have time

@simonbuchan
Copy link
Contributor Author

Thanks, I'll keep at it anyway. I'm pretty sure it's effectively the same command line (modulo order), so it's most likely something to do with current directory, variables, me using volta or the like. I just can't track down why that would result in what I'm seeing.

@simonbuchan
Copy link
Contributor Author

simonbuchan commented Nov 1, 2023

Ugh. Sorry for disappearing for like (checks) THREE WEEKS. Geez. I did poke at this a few times, but didn't figure out what could have been causing the last issue where cargo xtask auto-unignore was generating a bunch of .stc-stderr files (not .tsc-errors.json like I thought!)

Turned out to just be that auto-unignore.sh didn't set UPDATE=1. I have no idea why that makes a difference, there doesn't seem to be any reference to that variable in Rust source at least? Rather annoying that it was that simple.

And as I've said before, I'm happy to make any changes or additions you would prefer.

@simonbuchan simonbuchan changed the title WIP: chore: replace scripts with xtask pattern chore: replace scripts with xtask pattern Nov 1, 2023
@simonbuchan simonbuchan marked this pull request as ready for review November 1, 2023 07:30
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

I think we can remove the replaced scripts

tests.sort();

use std::io::Write;
let mut file = std::fs::File::create("crates/stc_ts_type_checker/tests/conformance.pass.txt")?;
Copy link
Member

@kdy1 kdy1 Nov 7, 2023

Choose a reason for hiding this comment

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

Should we use a buffer here?

@simonbuchan
Copy link
Contributor Author

Ok, I removed all the now replaced .sh files: there's still a few (like profile.sh) that should be pretty easy to replace now.

This updates the CI's use of the old scripts.. 🤞

For reference, the cargo xtask test-analyzer works fine on windows, but test-checker fails with:

...
error: test failed, to rerun pass `-p stc_ts_type_checker --test tsc`
Error: test conformance failed: exit code: 101
error: process didn't exit successfully: `target\debug\xtask.exe test-checker` (exit code: 1)

which I think is the mimalloc issue I referred to above? When I get a little bandwidth I'll see if I can track down what's going on there...

@simonbuchan
Copy link
Contributor Author

Hmm, cargo xtask test-checker did also panic on CI in "PR Maintenance / All passing tests should be enabled (pull_request) " ... might be something going on there? Not much detail.

It's currently expected to fail.

This also doesn't skip .tsx tests (free passing tests!) and prints
dot-style test output progress.
@simonbuchan
Copy link
Contributor Author

Ah, it's expected to fail for now, but also it fails with mimalloc heap corruption on windows.

I fixed it not updating conformance.pass.txt if it fails, and also tossed in a dot progress reporter and stopped it filtering out .tsx test passes. Let me know if either of those seem bad to you!

@kdy1
Copy link
Member

kdy1 commented Nov 14, 2023

It's fine, but CI failed.

@simonbuchan
Copy link
Contributor Author

simonbuchan commented Nov 14, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants