Converted open rule tests from c* to no-* #665

JoshuaKGoldberg · 2018-12-04T00:39:35Z

PR checklist

Addresses an existing issue: continues Switch from the custom unit test harness to TSLint's --test #489

Overview of change:

Excluding @IllusionMH's 🏃 rules, converts list items mentioned starting from chai-* through no-* rules.

Continues #489.

msftclas · 2018-12-04T00:39:48Z

All CLA requirements met.

tests/import-name/ignored-list-fail/tslint.json

src/noBackboneGetSetOutsideModelRule.ts

src/noSuspiciousCommentRule.ts

JoshuaKGoldberg · 2018-12-14T17:09:36Z

Ugh CRLF... trying to fix up one discrepancy...

.gitattributes

JoshuaKGoldberg · 2018-12-14T17:38:36Z

@IllusionMH I'll leave this open in case you want to review, then merge it in if you don't get a chance or in the next week. :)

IllusionMH · 2018-12-19T10:32:17Z

@JoshuaKGoldberg sorry for delay, I will return to reviewing this PR later today.

tests/function-name/default/test.ts.lint

IllusionMH

There are some duplicate blocks and folders. And looks like 1 valid case is missing in (at least I wasn't able to find it).

Will continue review tomorrow.

tests/import-name/ignored-list-fail/test.ts.lint

src/tests/MissingOptionalAnnotationTests.ts

tests/missing-jsdoc/three-star-comment - Copy/test.ts.lint

tests/no-banned-terms/test.ts.lint

IllusionMH

Found few tests that are in initial files, but not in new (however, some of them are debatable and can be skipped).

At this point I think I've finished review.

src/tests/NoConstantConditionRuleTests.ts

tests/no-http-string/exclude/tslint.json

src/tests/NoIncrementDecrementTests.ts

src/tests/NoRegexSpacesRuleTests.ts

tests/no-unexternalized-strings/signatures/test.ts.lint

tests/mocha-no-side-effect-code/multiline-string/test.ts.lint

tests/mocha-no-side-effect-code/configured-factory-methods/test.ts.lint

JoshuaKGoldberg · 2018-12-26T02:29:31Z

Thanks for the thorough review @IllusionMH! I believe I've addressed all your points. No rush on re-reviewing 😊

package-lock.json

IllusionMH

Most fixes for first review comments are in place, however few problems are still present.

Also found some problems that I've missed during initial review 😞
Also discovered few implementation problems. Will create follow up issues for them.

IllusionMH · 2019-01-07T00:13:16Z

src/chaiVagueErrorsRule.ts

 const FAILURE_STRING_COMPARE_FALSE: string =
-    BASE_ERROR + 'Move the strict inequality comparison from the expect call into the assertion value. ';
+    BASE_ERROR + 'Move the strict inequality comparison from the expect call into the assertion value';


Nit: dot was added to previous error message, but removed from this one. Nice to have them consistent.

IllusionMH · 2019-01-07T00:46:57Z

src/tests/FunctionNameRuleTests.ts

-        TestHelper.assertViolations(ruleName, script, []);
-    });
-
-    it('should pass on correctly private static methods', (): void => {


private static tests are missing from function-name/default/test.ts.lint

IllusionMH · 2019-01-07T00:47:20Z

src/tests/FunctionNameRuleTests.ts

-        TestHelper.assertViolations(ruleName, script, []);
-    });
-
-    it('should pass on correctly protected static methods', (): void => {


protected static tests are missing from function-name/default/test.ts.lint

IllusionMH · 2019-01-07T01:39:00Z

tests/function-name/validate-private-statics-as-static/tslint.json

@@ -0,0 +1,5 @@
+{
+    "rules": {
+        "function-name": [true, "validate-private-statics-as-static"]


Looks like string options like "validate-private-statics-as-static" are only checked as second option (where first one is object with RegExps).
Source and test that passes 'true' in place of object (first option) and 'validate-private-statics-as-static' as second option.

Should be

Suggested change

"function-name": [true, "validate-private-statics-as-static"]

"function-name": [true, {}, "validate-private-statics-as-static"]

And validate-private-statics-as-static/test.ts.lint is supposed to check that private static are valid when they are upper case. No errors in both test blocks.

Same problem with options position in validate-private-statics-as-private/tslint.json and validate-private-statics-as-either/tslint.json.

Probably related to #661

IllusionMH · 2019-01-08T02:25:16Z

tests/function-name/validate-private-statics-as-private/tslint.json

@@ -0,0 +1,5 @@
+{
+    "rules": {
+        "function-name": [true, "validate-private-statics-as-private"]


As described for validate-private-statics-as-static string param should be in second option position.

Suggested change

"function-name": [true, "validate-private-statics-as-private"]

"function-name": [true, {}, "validate-private-statics-as-private"]

IllusionMH · 2019-01-10T23:56:45Z

tests/no-control-regex/test.ts.lint

@@ -0,0 +1,18 @@
+var pattern1 = /\\x20/;


All regular expressions (except in string literals) should have single \ before x. Otherwise it is just escaped back-slash

IllusionMH · 2019-01-10T23:59:17Z

tests/no-control-regex/test.ts.lint

+var pattern1 = /\\x00/;
+               ~~~~~~~  [0]
+var pattern2 = new RegExp("\\x00");
+var pattern3 = RegExp("\\x00");


These 2 lines should have errors too according to old tests.
On the second thought - it is an issue with implementation based on forgot escapes for \ in strings that contain strings that should have escapes for \ (and we must go deeper 😂 ) in "string-based" Mocha tests.

Created #785 for further discussion

IllusionMH · 2019-01-11T01:51:49Z

tests/no-suspicious-comment/exceptions/tslint.json

@@ -0,0 +1,5 @@
+{
+    "rules": {
+        "no-suspicious-comment": [true, ["https://example.com/*"]]


Looks like RegExps shouldn't be additionally wrapped in array.
In original test all passed options are just array of strings

UPD. Looks like in this case it relies on ['123'].toString() === '123', but will fail if 2 or more strings passed

IllusionMH · 2019-01-11T02:05:59Z

src/tests/NoSuspiciousCommentRuleTests.ts

-
-        it(`should fail on lower case ${suspiciousWord} comments with colons`, (): void => {
-            const script: string = `
-                // ${suspiciousWord}: you should fix this


Looks like there were no actual tests for lowercase suspicious words 😂, because .toLowerCase() was missing.
It will be tested by case with link, but for consistency (with initial intent) simple case can be added too.

IllusionMH · 2019-01-11T02:23:39Z

src/tests/NoUnsupportedBrowserCodeRuleTests.ts

-import { Utils } from '../utils/Utils';
-import { TestHelper } from './TestHelper';
-
-describe('noUnsupportedBrowserCodeRule', (): void => {


Can't find new tests for this rule

JoshuaKGoldberg · 2019-01-14T02:28:20Z

Thanks for the ping - as we discussed, I'm going to close this one and open many smaller PRs for both of our sanities. For not the first time, lesson learned: giant PRs are almost never a good idea!

Converted open rules from a c* to no-*

ef82451

Continues #489.

JoshuaKGoldberg commented Dec 4, 2018

View reviewed changes

tests/import-name/ignored-list-fail/tslint.json Show resolved Hide resolved

JoshuaKGoldberg commented Dec 4, 2018

View reviewed changes

src/noBackboneGetSetOutsideModelRule.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg commented Dec 4, 2018

View reviewed changes

src/noSuspiciousCommentRule.ts Show resolved Hide resolved

Undid some unnecessary changes

734f485

JoshuaKGoldberg changed the title Converted open rules from a c* to no-* Converted open rule tests from a c* to no-* Dec 4, 2018

IllusionMH mentioned this pull request Dec 10, 2018

Enabled no-non-null-assertion internally #669

Merged

JoshuaKGoldberg changed the title Converted open rule tests from a c* to no-* Converted open rule tests from c* to no-* Dec 11, 2018

Josh Goldberg added 2 commits December 13, 2018 14:03

Fixed incorrect error markers

af81dbb

Undid package.json changes; testing rename of import-name/default...

143c72e

Josh Goldberg added 2 commits December 14, 2018 09:25

Deleted import-name...

a59f082

How about with .fix added to .gitattributes?

1d842c7

JoshuaKGoldberg commented Dec 14, 2018

View reviewed changes

.gitattributes Outdated Show resolved Hide resolved

IllusionMH added the PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! label Dec 19, 2018

IllusionMH reviewed Dec 21, 2018

View reviewed changes

tests/function-name/default/test.ts.lint Outdated Show resolved Hide resolved

IllusionMH reviewed Dec 21, 2018

View reviewed changes

tests/function-name/default/test.ts.lint Show resolved Hide resolved

IllusionMH suggested changes Dec 22, 2018

View reviewed changes

IllusionMH suggested changes Dec 23, 2018

View reviewed changes

IllusionMH added PR: Waiting for Author Changes have been requested that the pull request author should address. and removed PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! labels Dec 23, 2018

Addressed IllusionMH's feedback

e9ad36b

JoshuaKGoldberg added PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! and removed PR: Waiting for Author Changes have been requested that the pull request author should address. labels Dec 26, 2018

Added missing test.ts.fix

a1067dc

IllusionMH mentioned this pull request Jan 10, 2019

void-0-rule #770

Merged

4 tasks

IllusionMH mentioned this pull request Jan 10, 2019

Enforce line endings for .fix files #782

Merged

2 tasks

Merge branch 'master'

8d44f65

JoshuaKGoldberg commented Jan 10, 2019

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

Agh, package-lock

c5e1d56

IllusionMH suggested changes Jan 11, 2019

View reviewed changes

IllusionMH added PR: Waiting for Author Changes have been requested that the pull request author should address. and removed PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! labels Jan 11, 2019

IllusionMH mentioned this pull request Jan 14, 2019

no-control-regex doesn't catch erros on escaped strings #785

Closed

JoshuaKGoldberg closed this Jan 14, 2019

JoshuaKGoldberg deleted the more-rule-test-conversions branch January 14, 2019 02:28

IllusionMH mentioned this pull request Jan 14, 2019

function-name rule has mismatches in options schema/meta, readme description, and actually accepted rules #786

Closed

IllusionMH added this to the 6.1.0-beta milestone Feb 19, 2019

JoshuaKGoldberg mentioned this pull request Jun 20, 2020

refactor: ban-ts-ignore converter to ban-ts-comment typescript-eslint/tslint-to-eslint-config#616

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converted open rule tests from c* to no-* #665

Converted open rule tests from c* to no-* #665

JoshuaKGoldberg commented Dec 4, 2018

msftclas commented Dec 4, 2018 •

edited

Loading

JoshuaKGoldberg commented Dec 14, 2018

JoshuaKGoldberg commented Dec 14, 2018

IllusionMH commented Dec 19, 2018

IllusionMH left a comment

IllusionMH left a comment

JoshuaKGoldberg commented Dec 26, 2018

IllusionMH left a comment

IllusionMH Jan 7, 2019

IllusionMH Jan 7, 2019

IllusionMH Jan 7, 2019

IllusionMH Jan 7, 2019

IllusionMH Jan 14, 2019

IllusionMH Jan 8, 2019

IllusionMH Jan 10, 2019

IllusionMH Jan 10, 2019

IllusionMH Jan 14, 2019

IllusionMH Jan 11, 2019

IllusionMH Jan 11, 2019

IllusionMH Jan 11, 2019

JoshuaKGoldberg commented Jan 14, 2019

	"function-name": [true, "validate-private-statics-as-static"]
	"function-name": [true, {}, "validate-private-statics-as-static"]

Converted open rule tests from c* to no-* #665

Converted open rule tests from c* to no-* #665

Conversation

JoshuaKGoldberg commented Dec 4, 2018

PR checklist

Overview of change:

msftclas commented Dec 4, 2018 • edited Loading

JoshuaKGoldberg commented Dec 14, 2018

JoshuaKGoldberg commented Dec 14, 2018

IllusionMH commented Dec 19, 2018

IllusionMH left a comment

Choose a reason for hiding this comment

IllusionMH left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Dec 26, 2018

IllusionMH 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Jan 14, 2019

msftclas commented Dec 4, 2018 •

edited

Loading