AC-1156: Move custom eslint tests to magento-coding-standard repo #262

loginesta · 2021-09-07T13:57:46Z

https://jira.corp.magento.com/browse/AC-1156 Move custom eslint tests to magento-coding-standard repo

ihor-sviziev

Could you add a new GitHub actions workflow to run these eslint tests on top of this repo, similar to phpcs?

ihor-sviziev · 2021-09-08T03:25:51Z

Pls not forget moving the following changes

magento/magento2@f0f779a

loginesta · 2021-09-08T07:47:31Z

@ihor-sviziev Thanks for your comments! will take care of the proposed changes magento/magento2@f0f779a
Also, please note this is still WIP

sivaschenko

Thanks for the PR @loginesta ! Please see my comments

sivaschenko · 2021-09-14T12:31:22Z

eslint/Tests/EslintUnitTest.php

+            $result
+        );
+
+        assertStringContainsString($expectedMessage, $output[2]);


Please use the inherited methods instead of importing a function

Suggested change

assertStringContainsString($expectedMessage, $output[2]);

$this->assertStringContainsString($expectedMessage, $output[2]);

sivaschenko · 2021-09-14T12:32:13Z

eslint/Tests/EslintUnitTest.php

+        assertStringContainsString($expectedMessage, $output[2]);
+    }
+
+    public function testBindUnbind()


It would be good to extract the abstract testcase with common functionality and introduce a child testcase class for each rule

what about defining everything in an array and loop in?

foreach ($this->rules as $rule) { exec(self::command . __DIR__ . '/' . $rule['path'], $output, $result); $this->assertStringContainsString($rule['message'], $output[2]); }

sivaschenko · 2021-09-14T12:33:28Z

eslint/Tests/EslintUnitTest.php

+ * See COPYING.txt for license details.
+ */
+
+namespace Magento\User\Block\Role\Tab;


Please move the unit tests to correct the Magento2/Tests/Eslint folder and correct the namespace

sivaschenko · 2021-09-14T12:33:54Z

eslint/Tests/EslintUnitTest.php

+
+class EslintUnitTest extends \PHPUnit\Framework\TestCase
+{
+    const command = '../node_modules/.bin/eslint -c eslint/.eslintrc --rulesdir eslint/rules ';


Do not forget to specify access level

Suggested change

const command = '../node_modules/.bin/eslint -c eslint/.eslintrc --rulesdir eslint/rules ';

private const command = '../node_modules/.bin/eslint -c eslint/.eslintrc --rulesdir eslint/rules ';

sivaschenko · 2021-09-14T12:35:05Z

.github/workflows/php.yml

@@ -48,3 +48,6 @@ jobs:

    - name: Run code style suite
      run: vendor/bin/phpcs --standard=Magento2 Magento2/Helpers/ Magento2/Sniffs
+
+    - name: Run ESLint
+        run: ../node_modules/.bin/eslint -c eslint/.eslintrc --rulesdir eslint/rules eslint/rules


Needs additional step(s) to install eslint

sivaschenko · 2021-09-14T12:36:29Z

eslint/rules/jquery-no-trim.js

+    type: 'suggestion',
+    docs: {
+      description: 'Disallow the use of the deprecated `trim` function',
+      category: 'Possible Errors',


Can we use custom categorization?

Suggested change

category: 'Possible Errors',

category: 'jQuery deprecated functions',

I was actually thinking of a more customised name for category.
But, not sure about it since I found this: https://eslint.org/docs/rules/

discussed offline. proceeding to update with proposed changes

ihor-sviziev · 2021-09-14T14:56:28Z

.github/workflows/php.yml

@@ -50,4 +50,5 @@ jobs:
      run: vendor/bin/phpcs --standard=Magento2 Magento2/Helpers/ Magento2/Sniffs

    - name: Run ESLint
-        run: ../node_modules/.bin/eslint -c eslint/.eslintrc --rulesdir eslint/rules eslint/rules
+      uses: stefanoeb/eslint-action@1.0.2


looks like that's not the right choice
https://github.com/stefanoeb/eslint-action#%EF%B8%8F%EF%B8%8F%EF%B8%8F-you-dont-need-this-action-%EF%B8%8F%EF%B8%8F%EF%B8%8F

And one more - it doesn't make any sense running it on different PHP versions

Not sure if I need the line that is referring to yarn since it's something external to the project.
Or maybe npm install because we are actually using node_modules...
Anyway, I'm going to try to see if any of these work. Thanks @ihor-sviziev

loginesta · 2021-09-16T16:09:23Z

magento/magento2@f0f779a

@ihor-sviziev do you refer to changes on this file right?
dev/tests/static/testsuite/Magento/Test/Js/_files/eslint/.eslintrc
In order to use ./.eslintrc-misc instead of ./.eslintrc-tinymce

sivaschenko

Thanks for all the updates @loginesta !

Can you please update the readme with instructions on how to run the eslint tests.

Also, can you please squash the changes in this pull request to a single commit

sivaschenko · 2021-09-17T11:46:07Z

Magento2/Tests/Eslint/AbstractEslintTestCase.php

+
+abstract class AbstractEslintTestCase extends TestCase
+{
+    protected function assertFileContainsError(string $testFile, array $expectedMessages)


can you please add strict types and phpdocs to the added tests

should be done, please check it out!

sivaschenko · 2021-09-17T14:56:50Z

Magento2/Tests/Eslint/AbstractEslintTestCase.php

+     * @param string $testFile
+     * @param array $expectedMessages
+     */
+    protected function assertFileContainsError(string $testFile, array $expectedMessages)


Suggested change

protected function assertFileContainsError(string $testFile, array $expectedMessages)

protected function assertFileContainsError(string $testFile, array $expectedMessages): void

sivaschenko · 2021-09-17T14:57:20Z

Magento2/Tests/Eslint/AndSelfTest.php

+ */
+class AndSelfTest extends AbstractEslintTestCase
+{
+    public function testExecute()


And remaining functions as well

Suggested change

public function testExecute()

public function testExecute(): void

… repo - Fix typo in README and add missing 'void' on unit tests

loginesta · 2021-09-17T16:27:42Z

@magento import pr to magento-commerce/magento-coding-standard

m2-github-services · 2021-09-17T16:27:48Z

@loginesta an error occurred during the Pull Request import.

loginesta · 2021-09-17T16:30:16Z

@magento import pr to magento-commerce/magento-coding-standard

m2-github-services · 2021-09-17T16:30:20Z

@loginesta an error occurred during the Pull Request import.

loginesta · 2021-09-20T07:41:13Z

@magento import pr to magento-commerce/magento-coding-standard

m2-github-services · 2021-09-20T07:41:18Z

@loginesta the Pull Request is successfully imported.

ihor-sviziev · 2021-09-20T09:39:57Z

.github/workflows/php.yml

+    - name: Install dependencies
+      run: npm install
+    - name: Run ESLint
+      run: npm run eslint -- eslint/rules


I think it should be separated into a different workflow to run the tests on different nodejs versions, also I don't think we should run eslint tests on different PHP versions

ihor-sviziev · 2021-09-20T09:51:06Z

.github/workflows/php.yml

@@ -25,6 +25,9 @@ jobs:
    name: Tests with PHP ${{ matrix.php-version }} and ${{ matrix.dependencies }} dependencies

    steps:
+    - name: Setup node
+      uses: actions/setup-node@v2


From readme file:
https://github.com/actions/setup-node

m2-community-project bot added the Progress: ready for review label Sep 7, 2021

fredden mentioned this pull request Sep 7, 2021

Update ESLint configuration magento/magento2#32343

Closed

5 tasks

m2-community-project bot assigned ihor-sviziev Sep 7, 2021

ihor-sviziev requested changes Sep 7, 2021

View reviewed changes

m2-community-project bot added Progress: changes requested and removed Progress: ready for review labels Sep 7, 2021

loginesta marked this pull request as ready for review September 14, 2021 11:59

sivaschenko requested changes Sep 14, 2021

View reviewed changes

m2-community-project bot assigned sivaschenko Sep 14, 2021

sivaschenko reviewed Sep 14, 2021

View reviewed changes

m2-community-project bot added Progress: review and removed Progress: changes requested labels Sep 14, 2021

ihor-sviziev reviewed Sep 14, 2021

View reviewed changes

sivaschenko requested changes Sep 17, 2021

View reviewed changes

m2-community-project bot added Progress: changes requested and removed Progress: review labels Sep 17, 2021

m2-community-project bot added Progress: review and removed Progress: changes requested labels Sep 17, 2021

sivaschenko reviewed Sep 17, 2021

View reviewed changes

AC-1156: Move custom eslint tests to magento-coding-standard repo

f44af55

loginesta force-pushed the AC-1156 branch from bbe382b to f44af55 Compare September 17, 2021 15:02

sivaschenko previously approved these changes Sep 17, 2021

View reviewed changes

m2-community-project bot added Progress: approved and removed Progress: review labels Sep 17, 2021

AC-1156: AC-1156: Move custom eslint tests to magento-coding-standard…

71970da

… repo - Fix typo in README and add missing 'void' on unit tests

loginesta dismissed sivaschenko’s stale review via 71970da September 17, 2021 15:13

magento-devops-reposync-svc merged commit d552f6a into magento:develop Sep 20, 2021

m2-community-project bot added Progress: done and removed Progress: approved labels Sep 20, 2021

loginesta deleted the AC-1156 branch September 20, 2021 08:18

ihor-sviziev reviewed Sep 20, 2021

View reviewed changes

m2-community-project bot removed the Progress: done label Sep 20, 2021

ihor-sviziev reviewed Sep 20, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AC-1156: Move custom eslint tests to magento-coding-standard repo #262

AC-1156: Move custom eslint tests to magento-coding-standard repo #262

loginesta commented Sep 7, 2021

ihor-sviziev left a comment

ihor-sviziev commented Sep 8, 2021

loginesta commented Sep 8, 2021

sivaschenko left a comment

sivaschenko Sep 14, 2021

sivaschenko Sep 14, 2021

loginesta Sep 14, 2021

sivaschenko Sep 14, 2021

sivaschenko Sep 14, 2021

sivaschenko Sep 14, 2021

loginesta Sep 16, 2021

sivaschenko Sep 14, 2021

loginesta Sep 14, 2021

loginesta Sep 14, 2021

ihor-sviziev Sep 14, 2021 •

edited

Loading

ihor-sviziev Sep 14, 2021

loginesta Sep 14, 2021

loginesta commented Sep 16, 2021

sivaschenko left a comment •

edited

Loading

sivaschenko Sep 17, 2021

loginesta Sep 17, 2021

sivaschenko Sep 17, 2021

sivaschenko Sep 17, 2021

loginesta commented Sep 17, 2021

m2-github-services commented Sep 17, 2021

loginesta commented Sep 17, 2021

m2-github-services commented Sep 17, 2021

loginesta commented Sep 20, 2021

m2-github-services commented Sep 20, 2021

ihor-sviziev Sep 20, 2021

ihor-sviziev Sep 20, 2021

	assertStringContainsString($expectedMessage, $output[2]);
	$this->assertStringContainsString($expectedMessage, $output[2]);

	const command = '../node_modules/.bin/eslint -c eslint/.eslintrc --rulesdir eslint/rules ';
	private const command = '../node_modules/.bin/eslint -c eslint/.eslintrc --rulesdir eslint/rules ';

	category: 'Possible Errors',
	category: 'jQuery deprecated functions',

	protected function assertFileContainsError(string $testFile, array $expectedMessages)
	protected function assertFileContainsError(string $testFile, array $expectedMessages): void

	public function testExecute()
	public function testExecute(): void

AC-1156: Move custom eslint tests to magento-coding-standard repo #262

AC-1156: Move custom eslint tests to magento-coding-standard repo #262

Conversation

loginesta commented Sep 7, 2021

ihor-sviziev left a comment

Choose a reason for hiding this comment

ihor-sviziev commented Sep 8, 2021

loginesta commented Sep 8, 2021

sivaschenko 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

ihor-sviziev Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loginesta commented Sep 16, 2021

sivaschenko left a comment • 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

loginesta commented Sep 17, 2021

m2-github-services commented Sep 17, 2021

loginesta commented Sep 17, 2021

m2-github-services commented Sep 17, 2021

loginesta commented Sep 20, 2021

m2-github-services commented Sep 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihor-sviziev Sep 14, 2021 •

edited

Loading

sivaschenko left a comment •

edited

Loading