Added new rule to detect assertions where statusCode is in the parameter #97

jpolavar · 2024-11-25T16:36:45Z

Closes #96

Tested with internal services

le-cong

just a few things, also does it make sense to check number literal values such as 200/409/etc. because they are also frequently used in status code assertion? ideally they should be replaced by StatusCodes.OK/etc. from http-status-codes package.
maybe it should be a separated rules? i'm not sure ...

le-cong · 2024-12-05T15:49:26Z

src/no-status-code-assert.ts

+
+export const ruleId = 'no-status-code-assert';
+const NO_STATUS_CODE_ASSERT = 'NO_STATUS_CODE_ASSERT';
+const keywords = ['status', 'code', 'StatusCodes', 'statusCode'];


status/code are widely used in none response status code related assertions, e.g. things like account.status/message.code can be incorrectly labelled.

@le-cong refactored to just check for StatusCodes for all assertions in parameters else if it is BinaryExpression check on either side.

le-cong · 2024-12-05T15:50:33Z

src/no-status-code-assert.ts

+ */
+const hasStatusCodeOrValue = (arg: TSESTree.Node): boolean => {
+  const checkName = (name?: string): boolean =>
+    name !== undefined && keywords.some((keyword) => name.toLowerCase().includes(keyword));


the usage of name.toLowerCase() seems contradict with the keyword const values which contains upper case values.

le-cong

just a few minor items

le-cong · 2024-12-23T18:07:20Z

src/no-status-code-assert.ts

+  node.object.name === 'assert' &&
+  node.property.type === AST_NODE_TYPES.Identifier;
+
+//Checks if a given AST node is an identifier with the name 'assert'.


le-cong · 2024-12-23T18:10:41Z

src/no-status-code-assert.ts

+
+const isAssertCallWithStatusCode = (callee: TSESTree.Node, args: TSESTree.Node[]): boolean =>
+  (isAssertIdentifier(callee) || isAssertMemberExpression(callee)) &&
+  args.some((arg) => hasStatusCodeOrValue(arg) && arg.type !== AST_NODE_TYPES.Identifier);


the arg.type check seems unnecessary given the fact that hasStatusCodeOrValue function is already testing some specific conditions in its switch statements.

le-cong · 2024-12-23T18:32:32Z

src/no-status-code-assert.ts

+  switch (arg.type) {
+    case AST_NODE_TYPES.Literal:
+      if (typeof arg.value === 'number') {
+        return isStatusCodeLiteral(arg.value);


this branch doesn't have test coverage, either add test case or remove this logic and leave it for a separated rule to detect the direct usage of numeric value instead of http-status-codes for response status code?

oh we are just focusing on statusCodes, we can remove this

le-cong · 2024-12-23T19:45:07Z

docs/rules/no-status-code-assert.md

+## Fail
+
+```js
+assert(statusCode === 200);


update to reflect the latest implementation?

@carlansley should we have rule to auto fix the prod code with statusCode if we have numbers?

Not sure I can answer that, can we add a little more description to the issue/PR about what this entire rule is needed for? I'm sure this was discussed at some point but tbh I don't recall what the original problem was.

We need to monitor for assertion failures in production and also improve how assertions are used in production code. Currently, assertions are being used for status checks, which isn’t ideal. A good first step is to implement an ESLint rule that will detect when assertions are made using a statusCode as one of the parameters. This will help identify and clean up improper use of assertions in the code. notify service could look for assertion errors, but we need to clean up usage of assertions for status checks.

As we are checking for statusCode, should we have another rule to auto fix the prod code with statusCode if we have numbers some thing like
assert(statusCode === 200);
would be assert(statusCode === StatusCodes.OK);

is there a mistake in the documentation wrt what is "pass" and what is "fail"? That was what confused me initially.

But initially it was like

Fail

assert(statusCode === 200); assert.equal(statusCode, 200);

Pass

assert(response.status === 200); assert.equal(response.status, 200);

Updated the rule to handle just statusCodes.

Fail

assert(statusCode === StatusCodes.OK); assert.equal(statusCode, StatusCodes.OK);

Pass

assert(response.status === 200); assert.equal(response.status, 200);

Pass wouldn't report as StatusCodes aren't used. This was the reason if we need to have another rule to auto fix all the numbers to statusCodes?

ramaghanta · 2025-01-06T19:30:25Z

docs/rules/no-status-code-assert.md

+assert(response.status === 200);
+assert.equal(response.status, 200);


shouldn't these be failures as well? In a nutshell, we want to disallow the usage of assert to check for the desired response status, whether by using status or statusCode, in production code.

status/code are widely used in none response status code related assertions, e.g. things like account.status/message.code can be incorrectly labelled. Should we check .status, .code in the params of assert instead

not sure I understand, but all I'm saying is, just like how you're disallowing assert.ok(response.statusCode === StatusCodes.OK), we should also disallow assert.ok(response.status === StatusCodes.OK)

assert.ok(response.status === StatusCodes.OK) is disallowed and would be reported as error but assert.equal(response.status, 200); is allowed since the rule check is going by StatusCodes. We can add that to disallow

created #109 to address the usage of magic number instead of StatusCodes.xxx

@le-cong, why do we need a separate rule? if 200/201/400/500 etc., are used, it will fail with a no-magic-numbers correct?

good point, the only thing is that no-magic-number rule is turned off for test codes, thought we might want to be consistent with status code handling all across targeting a specific subset of numbers. thoughts?

no-magic-number is turned off for https://github.com/checkdigit/eslint-config/blob/main/index.mjs#L208. So, the only HTTP response status codes that will be allowed would be 256 and 512, which are uncommon and I can't think of any services that use these.

L208 is for default setting including production code, but it's overridden by the next section in https://github.com/checkdigit/eslint-config/blob/5eea1c576703f1ba1663bd6bb73542d19ccbce5d/index.mjs#L438, which disables this rule completely for test codes.

got it. the new GitHub issue you created makes sense to me then

ramaghanta

lftm

carlansley

lftm, but main needs to be merged in

github-actions · 2025-01-08T21:17:26Z

❌ PR review status - not all reviewers have approved - 2 approved - 1 outstanding

github-actions · 2025-01-08T22:23:18Z

Coverage after merging no-status-assert-rule into main will be

85.05%	▴ +0.40%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
src
file-path-comment.ts	100%	100%	100%	100%
get-documentation-url.ts	100%	100%	100%	100%
index.ts	0%	0%	0%	0%	1, 1, 10, 100–109, 11, 110–119, 12, 120–129, 13, 130–139, 14, 140–145, 15–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–69, 7, 70–79, 8, 80–89, 9, 90–99
invalid-json-stringify.ts	100%	100%	100%	100%
no-card-numbers.ts	95.11%	89.29%	100%	96.03%	134–139, 144–146
no-duplicated-imports.ts	100%	100%	100%	100%
no-enum.ts	100%	100%	100%	100%
no-legacy-service-typing.ts	100%	100%	100%	100%
no-promise-instance-method.ts	100%	100%	100%	100%
no-random-v4-uuid.ts	100%	100%	100%	100%
no-serve-runtime.ts	100%	100%	100%	100%
no-side-effects.ts	99.54%	97.83%	100%	100%	132
no-status-code-assert.ts	100%	100%	100%	100%
no-test-import.ts	100%	100%	100%	100%
no-uuid.ts	95.16%	85.71%	100%	96.23%	100–101, 89–91, 99
no-wallaby-comment.ts	97.50%	85%	100%	100%	43–44, 58
object-literal-response.ts	100%	100%	100%	100%
regular-expression-comment.ts	95.51%	89.47%	100%	97.06%	35–37, 54
require-assert-predicate-rejects-throws.ts	100%	100%	100%	100%
require-fixed-services-import.ts	100%	100%	100%	100%
require-resolve-full-response.ts	83.69%	78.33%	100%	84.72%	105–107, 109–111, 113–115, 122–124, 127, 129, 129–131, 147–149, 199–210, 46, 65, 65–67, 81–83, 94–99
require-strict-assert.ts	98.40%	92%	100%	100%	44–45
require-ts-extension-imports-exports.ts	100%	100%	100%	100%
require-type-out-of-type-only-imports.ts	100%	100%	100%	100%
tester.test.ts	100%	100%	100%	100%
ts-tester.test.ts	100%	100%	100%	100%
src/library
format.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20, 3–9
tree.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–69, 7, 70–79, 8, 80–89, 9, 90–97
ts-tree.ts	46.09%	46.15%	37.50%	46.73%	100–107, 27–29, 29–30, 33, 33–35, 35–37, 42–43, 46–47, 56–74, 77–96, 99
variable.ts	0%	0%	0%	0%	1, 1–5

github-actions · 2025-01-08T22:23:31Z

Beta Published - Install Command: npm install @checkdigit/[email protected]

Added new rule to detect assertions where statusCode is in the parameter

efc924c

jpolavar added the MINOR label Nov 25, 2024

jpolavar self-assigned this Nov 25, 2024

Updated description

7c24b6b

jpolavar requested review from carlansley and le-cong November 25, 2024 17:17

jpolavar added 2 commits November 25, 2024 12:01

Updated rule messages

cf98fa1

Updated rule messages

6a0fa5b

le-cong requested changes Dec 5, 2024

View reviewed changes

jpolavar added 5 commits December 5, 2024 13:11

updated PR comments

77b0ed7

Added a valid test case

d7e6061

Merge from main

4a2a2a9

Updated to report errors for any StatusCodes in asserts

7681452

Updated to report errors for any StatusCodes in asserts

fb7a181

jpolavar requested a review from le-cong December 20, 2024 05:27

jpolavar added 2 commits December 19, 2024 23:39

Updated to report errors for any StatusCodes in asserts

0ac0fef

Merged latest changes from main

2ab6ac9

le-cong requested changes Dec 23, 2024

View reviewed changes

Remove unused numeric codes check

4ff304a

jpolavar requested a review from le-cong December 23, 2024 19:09

le-cong requested changes Dec 23, 2024

View reviewed changes

Remove unused numeric codes check

d626c8f

jpolavar requested a review from le-cong December 23, 2024 20:04

le-cong approved these changes Dec 23, 2024

View reviewed changes

carlansley requested a review from ramaghanta January 2, 2025 21:31

merged conflicts

2306703

ramaghanta requested changes Jan 6, 2025

View reviewed changes

Added functionality to report error if found any numeric status codes

2590d5e

jpolavar requested a review from ramaghanta January 6, 2025 23:13

Added functionality to report error if found any numeric status codes

8855abb

ramaghanta approved these changes Jan 7, 2025

View reviewed changes

carlansley requested changes Jan 8, 2025

View reviewed changes

jpolavar added 2 commits January 8, 2025 15:30

Merge branch 'main' into no-status-assert-rule

7b575ed

Merged main

2af864a

jpolavar requested a review from carlansley January 8, 2025 21:55

Merged main

d238a68

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new rule to detect assertions where statusCode is in the parameter #97

Added new rule to detect assertions where statusCode is in the parameter #97

jpolavar commented Nov 25, 2024 •

edited

Loading

le-cong left a comment

le-cong Dec 5, 2024

jpolavar Dec 20, 2024

le-cong Dec 5, 2024

le-cong left a comment

le-cong Dec 23, 2024

le-cong Dec 23, 2024

le-cong Dec 23, 2024

jpolavar Dec 23, 2024 •

edited

Loading

le-cong Dec 23, 2024

jpolavar Dec 24, 2024 •

edited

Loading

carlansley Dec 24, 2024

jpolavar Dec 24, 2024 •

edited by carlansley

Loading

carlansley Dec 24, 2024

jpolavar Dec 24, 2024 •

edited

Loading

ramaghanta Jan 6, 2025

jpolavar Jan 6, 2025

ramaghanta Jan 6, 2025

jpolavar Jan 6, 2025 •

edited

Loading

le-cong Jan 7, 2025

ramaghanta Jan 7, 2025

le-cong Jan 7, 2025

ramaghanta Jan 7, 2025

le-cong Jan 7, 2025

ramaghanta Jan 7, 2025

ramaghanta left a comment

carlansley left a comment

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

		assert(response.status === 200);
		assert.equal(response.status, 200);

Added new rule to detect assertions where statusCode is in the parameter #97

Are you sure you want to change the base?

Added new rule to detect assertions where statusCode is in the parameter #97

Conversation

jpolavar commented Nov 25, 2024 • edited Loading

le-cong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

le-cong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpolavar Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpolavar Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpolavar Dec 24, 2024 • edited by carlansley Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpolavar Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Fail

Pass

Fail

Pass

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpolavar Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaghanta left a comment

Choose a reason for hiding this comment

carlansley left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jpolavar commented Nov 25, 2024 •

edited

Loading

jpolavar Dec 23, 2024 •

edited

Loading

jpolavar Dec 24, 2024 •

edited

Loading

jpolavar Dec 24, 2024 •

edited by carlansley

Loading

jpolavar Dec 24, 2024 •

edited

Loading

jpolavar Jan 6, 2025 •

edited

Loading