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

Add snapshot tests for plugins #165

Merged
merged 16 commits into from
Sep 14, 2023
Merged

Add snapshot tests for plugins #165

merged 16 commits into from
Sep 14, 2023

Conversation

paul-sachs
Copy link
Collaborator

@paul-sachs paul-sachs commented Sep 13, 2023

Update CI to check for untracked files (as suggested by @smaye81). First PR part of #160.

Few minor adjustments

  • fixed a missing (optional) semi-colon in js generation
  • added prettier formatting for root level files

packages/protoc-gen-connect-query/snapshots/buf.gen.yaml Outdated Show resolved Hide resolved
packages/protoc-gen-connect-query/snapshots/buf.gen.yaml Outdated Show resolved Hide resolved
packages/protoc-gen-connect-query/snapshots/buf.gen.yaml Outdated Show resolved Hide resolved
packages/protoc-gen-connect-query/package.json Outdated Show resolved Hide resolved
@@ -1,4 +1,4 @@
{
"extends": "./tsconfig.build.json",
"include": ["src"]
"include": ["src", "snapshots"]
Copy link
Member

Choose a reason for hiding this comment

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

Good call 👍

Since we're generating both ts and dts - which one will be compiled though? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is really only useful for eslint so it can compile those files. It's a little weird and more finicky than I'd like.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is actually also just for the IDE + test + lint, whereas tsconfig.build.json is used for the build itself.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. What I mean is that there is ambiguity in general when you have x.js, x.ts, and x.d.ts all in the same directory, for any tool that processes the files.

I just checked with npm exec eslint -- snapshots/gen/connectrpc/eliza/v1 --debug, and see that eslint does lint each file individually.

It would not have caught #166 though.

That's fine for now - the "snapshots" will make changes reviewable, which is a big improvement.

Cut connectrpc/connect-es#824, because we don't do better there either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, got confused by github and replied down below. This change should catch any TS leaking into the javascript, but we probably need a more dedicated way to test these kinds of things (because we don't know which will be imported, as you mentioned).

packages/protoc-gen-connect-query-react/.eslintignore Outdated Show resolved Hide resolved
packages/protoc-gen-connect-query/snapshots/buf.gen.yaml Outdated Show resolved Hide resolved
@@ -1,4 +1,4 @@
{
"extends": "./tsconfig.build.json",
"include": ["src"]
"include": ["src", "snapshots"]
Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. What I mean is that there is ambiguity in general when you have x.js, x.ts, and x.d.ts all in the same directory, for any tool that processes the files.

I just checked with npm exec eslint -- snapshots/gen/connectrpc/eliza/v1 --debug, and see that eslint does lint each file individually.

It would not have caught #166 though.

That's fine for now - the "snapshots" will make changes reviewable, which is a big improvement.

Cut connectrpc/connect-es#824, because we don't do better there either.

@paul-sachs
Copy link
Collaborator Author

It would not have caught #166 though.

Aww man, I actually had triggered an error and caught the as const thing for a bit, something must've changed in my linting. I still want to make sure our lint step can catch that kind of thing. Will investigate why it's not catching it.

@paul-sachs
Copy link
Collaborator Author

@timostamm Ah ok, my testing was just flawed (changing the wrong plugin). The linter does catch #166:

@connectrpc/protoc-gen-connect-query:format: snapshots/gen/connectrpc/eliza/v1/eliza-ElizaService_connectquery.d.ts 33ms
@connectrpc/protoc-gen-connect-query:format: [error] snapshots/gen/connectrpc/eliza/v1/eliza-ElizaService_connectquery.js: SyntaxError: Missing semicolon. (76:2)
@connectrpc/protoc-gen-connect-query:format: [error]   74 |     },
@connectrpc/protoc-gen-connect-query:format: [error]   75 |   }
@connectrpc/protoc-gen-connect-query:format: [error] > 76 | } as const
@connectrpc/protoc-gen-connect-query:format: [error]      |  ^
@connectrpc/protoc-gen-connect-query:format: [error]   77 |
@connectrpc/protoc-gen-connect-query:format: [error]   78 | /**
@connectrpc/protoc-gen-connect-query:format: [error]   79 |  * Say is a unary RPC. Eliza responds to the prompt with a single sentence.

@timostamm
Copy link
Member

@timostamm Ah ok, my testing was just flawed (changing the wrong plugin).

And I ran pnpm exec eslint -- . to verify, which doesn't catch it for whatever reason...

@paul-sachs paul-sachs merged commit 40db627 into main Sep 14, 2023
4 checks passed
@paul-sachs paul-sachs deleted the psachs/ci-check-dirty branch September 14, 2023 20:42
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.

None yet

3 participants