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

feat: doctor checks for "set -e" in test scripts #207

Merged
merged 13 commits into from
Dec 10, 2024
Merged

Conversation

turadg
Copy link
Member

@turadg turadg commented Dec 7, 2024

Closes #206

tooling improvements

As long as it passes CI it should be fine.

I also tested locally with,

  • yarn build in the package
  • packages/synthetic-chain/dist/cli/cli.js doctor

That led to errors in many test.sh files so I cleaned those up.

I tried adopting the -e across all scripts but CI failed and I had to revert. We'll have them going forward.

@turadg turadg requested review from dckc and samsiegart December 7, 2024 02:01
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

works for me.

@@ -2145,6 +2164,22 @@ __metadata:
languageName: node
linkType: hard

"glob@npm:^11.0.0":
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised we don't already have a glob dep...

oh... this is in agoric-3-proposals.

In agoric-sdk, we have a menagerie:

$ grep 'glob ' yarn.lock 
    glob "8.0.3"
    glob "8.0.3"
    glob "8.0.3"
    glob "^8.0.1"
    glob "^7.1.6"
    is-glob "^4.0.3"
    glob "^7.1.6"
    fast-glob "^3.3.2"
    is-glob "^4.0.3"
    glob "^8.0.1"
    glob "^10.2.2"
    is-glob "~4.0.1"
    is-glob "^4.0.3"
    is-glob "^4.0.0"
    is-glob "^4.0.1"
    is-glob "^4.0.3"
    dir-glob "^3.0.1"
    fast-glob "^3.2.9"
    dir-glob "^3.0.1"
    fast-glob "^3.2.11"
    is-glob "^4.0.1"
    is-extglob "^2.1.1"
    glob "^7.1.4"
    glob "^8.0.1"
    fast-glob "3.2.7"
    glob "7.1.4"
    glob "^8.0.1"
    glob "^7.1.3"
    glob "^10.0.0"
    glob "^7.1.3"
    glob "^10.3.7"
    glob "^7.0.0"
    glob "^7.1.4"
    glob "^10.4.1"
    fast-glob "3"

@@ -1,4 +1,5 @@
#!/bin/bash
set -e
Copy link
Member

Choose a reason for hiding this comment

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

docs say...

"set -euo pipefail" is recommended.

you didn't want to risk compat problems?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, but I should follow the advice. updated.

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 had to revert it to get to green. Something for another time.

@@ -8,6 +8,7 @@ and then fulfill the remaining checklist items before merging.
Closes #

# adopt a passed proposal

Copy link
Member

Choose a reason for hiding this comment

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

is the prettier style set up to stick?

or was this just a 1-time?

Copy link
Member Author

@turadg turadg Dec 9, 2024

Choose a reason for hiding this comment

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

thanks for inquiring. It's one time and I didn't know why CI hadn't errored on it. Looks like --write makes --check only warn because it's updating: https://github.com/Agoric/agoric-3-proposals/actions/runs/12125081638/job/33804228478

I'll fix it

some deps had incompatible defs

Error: ../../node_modules/@types/glob/index.d.ts(29,42): error TS2694: Namespace '"/home/runner/work/agoric-3-proposals/agoric-3-proposals/node_modules/minimatch/dist/cjs/index"' has no exported member 'IOptions'.
solves package export map for cosmic-proto
@turadg turadg merged commit ce1df1d into main Dec 10, 2024
2 checks passed
@turadg turadg deleted the 206-check-test.sh branch December 10, 2024 14:40
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.

update doctor command to check for good test.sh
3 participants