test: clean protocol tests #4479
Conversation
|
Merging to
|
|
Q: The generated |
Yeah, these are in the protobuf files, which is all codegen'd. Those are still in the protobuf because old SDK versions can still submit to our RPC server. I don't believe there is a way to filter them at codegen, but that would be the only place to remove them. I don't really want them to have tests because that indicates "support" to me. |
protocol/client.ts
Outdated
| // body: details.body, | ||
| extra: details.extra, | ||
| email: typeof details.email === "string" ? details.email : undefined, | ||
| email: details.email, |
There was a problem hiding this comment.
I think we want defensive programming around this still.
There was a problem hiding this comment.
All the other properties pass things through. Why is email different from extra, query, etc?
Should those then not also get such checks?
There was a problem hiding this comment.
All properties besides email are provided via an adapter. However, email has to be passed to protect() because it is dynamic content, such as:
const decision = await aj.protect(req, { email: "foobarbaz" })Since type magic is done for the 2nd argument of protect (required values depending on configured rules), we'd need to do a lot of work to guard around it in adapters. It is just easier to guard this on the string type here and send undefined to the server, where validation will fail if they pass a number or something.
There was a problem hiding this comment.
I think this can be improved in the types then.
It does feel a bit smelly that there is but one such field.
Done.
protocol/client.ts
Outdated
| // body: details.body, | ||
| extra: details.extra, | ||
| email: typeof details.email === "string" ? details.email : undefined, | ||
| email: details.email, |
| // TODO(@wooorm-arcjet): | ||
| // if it is intentional that people can extend rules, | ||
| // then we need to allow that in the types. |
There was a problem hiding this comment.
This is allowed (see https://docs.arcjet.com/blueprints/defining-custom-rules) but is pretty advanced functionality. The types represent it so we have to consider in code that consumes rules.
There was a problem hiding this comment.
Cool!
It is possible to let users add types. I think that could be used here.
Similar to how TS adds things in more recent JavaScript versions.
As an example: MDX expressions are an extension to markdown. In the AST, they are added with the declare module stuff at the bottom of index.d.ts here: https://www.npmjs.com/package/mdast-util-mdx-expression?activeTab=code. It adds “into” the types at index.d.ts of https://www.npmjs.com/package/@types/mdast?activeTab=code
There was a problem hiding this comment.
I'm not against the idea, but I'd need to see it. I'd rather have simple types than exact types since many users just use JS with no type hinting and saying this can be any string helps code towards that.
There was a problem hiding this comment.
I am hopeful we can have both
protocol/test/client.test.ts
Outdated
| return new DecideResponse({ | ||
| decision: { | ||
| conclusion: Conclusion.ALLOW, | ||
| // TODO(@wooorm-arcjet): is it intentional that this is an *empty* rule, not like `rule` at all? |
There was a problem hiding this comment.
Welcome to the world of protobuf, where everything is optional and you hate your life 🙃
There was a problem hiding this comment.
Still, on L266, there are all these properties? Shouldn’t some of those fields go into this rule?
There was a problem hiding this comment.
An ArcjetRule and the Rule in the Protobuf are different. An ArcjetRule contains both the local and remote definitions, we use some of the properties to build the remote definition (when convert functions to protobuf). Since all fields in the Protobuf are optional, everything has a "zero" value and the constructor zeros every property—there is no reason to specify zero properties.
There was a problem hiding this comment.
okay, removed the note
protocol/test/client.test.ts
Outdated
| assert.equal(calls, 0); | ||
| calls++; | ||
|
|
||
| // TODO(@wooorm-arcjet): is it intentional that this is an *empty* rule, not like `rule` at all? |
|
Does this also resolve #32 ? |
I think so! I think you had a comment similar to that issue in the code, and I solved it, by sort of sidestepping it by inspecting |
|
I think that’s all the comments! |
In GH-4415 and GH-4472 I noticed that in particular the protocol tests could use some cleaning up.
I had some extra time to work on a big PR due to holiday in US.
Test cases were often testing equality of big objects as a sort of “fixture” testing. This made it difficult to see what was different between test cases. And, if a property would be added to such objects, all tests would need changing.
After cleaning (the first 2 commits), I went through the uncovered lines and added tests cases for them.
Importantly, there are barely any changes to
client.tsandindex.ts. Only because their behavior was not tested, and not allowed by the types, I removed the dead code.Please open the files that GH collapses, and tip: go through commits one by one