Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add justfile, remove coveralls, tweak ESLint #2249

Merged
merged 12 commits into from
Jan 16, 2025
Merged

Conversation

xavdid-stripe
Copy link
Member

Why?

In an effort to modernize and simplify our local tooling, we're moving our dev commands from makefiles to justfiles. This is intended to be mostly a drop-in replacement, but some command names may change per standardization efforts.

To be in line with our other SDKs, I removed most of the scripts in package.json. But, if we find that potential contributors are having trouble with muscle memory, we can re-add common ones (test, lint, etc) that call into just.

In order to untangle formatting and linting (which Prettier recommends keeping separate, I removed plugin:prettier from the eslint config. This mean the formatting-related lint rules are disabled, deferring to prettier instead. This has the added bonus of formatting violations not showing up as linting errors in-editor!

Lastly, I've moved the integration test suite out of the main test suite. It starts a bunch of processes that don't get canceled (eating up CPU locally). So now they're run only in CI by default (and via the just integrations-test, if you really want).

What?

  • add justfile
  • remove coveralls
  • tweak CI to use just
  • move prettier out of eslint
  • add a prettierignore (since prettier no longer gets to piggyback on eslint's file selection and we're on a super old version that doesn't respect .gitignore)
  • update README

See Also

DEVSDK-2325

@xavdid-stripe xavdid-stripe requested a review from a team as a code owner January 11, 2025 00:14
@xavdid-stripe xavdid-stripe requested review from prathmesh-stripe and removed request for a team January 11, 2025 00:14
# ⭐ run unit tests
[positional-arguments]
test *args: build
mocha "$@"
Copy link
Member Author

Choose a reason for hiding this comment

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

This was surprisingly tricky get right! By default, just groups all the args you pass to *args, so it wasn't correctly preserving -g "some thing". This works as expected for mixed content

Copy link
Contributor

@jar-stripe jar-stripe Jan 16, 2025

Choose a reason for hiding this comment

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

why not use *args? or are the args represented in both *args and $@? and if thats true, what happens if test is called from another just rule?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of the way just expands input args. It treats them as one big group for quoting purposes, unless told otherwise. It becomes an issue when some of your args are quoted with spaces and some aren't. You can't quote the whole thing, but you also can't leave them unquoted. This is important when wanting to call mocha someFile.js -g "match this name". If the args aren't grouped correctly when passing through just, you call -g match and then this and name are extra args.

I did some testing with the following:

[positional-arguments]
arg-test *args:
    bash -c 'while (( "$#" )); do echo - $1; shift; done' -- "{{ args }}"

Which should print each arg (respecting spaces) on their own line.

"{{ args }}":

% .j arg-test a b "c d" e
- a b c d e

{{ args }}:

- a
- b
- c
- d
- e

"$@":

% .j arg-test a b "c d" e
- a
- b
- c d
- e

I mostly pulled from the docs: https://github.com/casey/just?tab=readme-ov-file#positional-arguments

@@ -4,6 +4,15 @@ import {FAKE_API_KEY} from './testUtils.js';
const nodeVersion = parseInt(process.versions.node.split('.')[0], 10);

describe('Integration test', function() {
// these tests are expensive and start processes they don't clean up
Copy link
Member Author

Choose a reason for hiding this comment

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

This is why your CPU goes to 100% when running the test suite locally!

Copy link
Contributor

Choose a reason for hiding this comment

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

is there a fix we can make here? or is it more complicated than can be fixed as part of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's fixable, but IMO out of scope of this PR. I didn't want to get too distracted. Plus we don't care if we're leaking on CI, since it all gets shut down when the job completes

@xavdid-stripe xavdid-stripe enabled auto-merge (squash) January 16, 2025 02:24
@xavdid-stripe xavdid-stripe enabled auto-merge (squash) January 16, 2025 18:52
Copy link
Contributor

@jar-stripe jar-stripe left a comment

Choose a reason for hiding this comment

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

Looks good, and I love the paper trail we've left here !

@xavdid-stripe xavdid-stripe merged commit 055bfff into master Jan 16, 2025
9 checks passed
@xavdid-stripe xavdid-stripe deleted the DEVSDK-2325 branch January 16, 2025 23:06
gurus00 pushed a commit to gurus00/stripe-node that referenced this pull request Feb 11, 2025
* add justfile, remove coveralls

* fix ci

* fix ci

* better CI Logging

* now come on

* clean up CI and make more reliable

* update readme instructions

* update justfile import

* try pinning to ubuntu 24.04

* clean up CI

* split out recipes
gurus00 pushed a commit to gurus00/stripe-node that referenced this pull request Feb 11, 2025
* add justfile, remove coveralls

* fix ci

* fix ci

* better CI Logging

* now come on

* clean up CI and make more reliable

* update readme instructions

* update justfile import

* try pinning to ubuntu 24.04

* clean up CI

* split out recipes
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants