Updating Tests to latest Prettier tests #448

Janther · 2021-03-14T03:55:33Z

there are a few benefits of updating our testing framework.

the new snapshots give more details.
the FULL_TEST not only compares AST but also different EOL, BOM, and compares the file formatted twice (as described in Prettier's documentation) which led to the discovery with Assembly Labels adding an extra line if there is an existing one.

codecov · 2021-03-14T03:57:31Z

Codecov Report

Merging #448 (c894090) into master (9ef89c4) will not change coverage.
The diff coverage is n/a.

❗ Current head c894090 differs from pull request most recent head c9f50a3. Consider uploading reports for the commit c9f50a3 to get more accurate results

@@            Coverage Diff            @@
##            master      #448   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           92        92           
  Lines          770       770           
  Branches       147       147           
=========================================
  Hits           770       770

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ef89c4...c9f50a3. Read the comment docs.

Janther · 2021-03-14T04:08:02Z

tests/EnumDefinitions/__snapshots__/jsfmt.spec.js.snap

@@ -1,13 +1,20 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP

-exports[`EnumDefinitions.sol 1`] = `
+exports[`EnumDefinitions.sol - {"bracketSpacing":true} format 1`] = `


things like these are very nice when reviewing a snapshot with specific options

Janther · 2021-03-14T04:08:49Z

tests/ExplicitVariableTypes/__snapshots__/jsfmt.spec.js.snap

-        uint256 _e,
-        int256 _f,
-        bytes1 _g
+        uint _a,


These changed because the order of the snapshots changed.

tests_config/run_spec.js

Janther · 2021-03-14T04:11:39Z

tests_config/run_spec.js

+
+// TODO: these test files need fix
+const unstableTests = new Map(
+  ['Assembly/Assembly.sol'].map((fixture) => {


Until we fix this Issue we won't compare this file formatted twice.

mattiaerre · 2021-03-17T03:37:59Z

happy to approve this if you can fix the conflicts @Janther thanks

Janther · 2021-03-17T03:56:00Z

AFK right now but the conflict might only be with the lock file since the merge of the eslint update

mattiaerre · 2021-03-17T19:32:54Z

thanks @Janther this is a 104 files long PR, anything specific you'd like us to take a look at?

Janther · 2021-03-17T20:01:17Z

Every jsfmt.spec.js in the test folder had the parser included in the call to run_spec.
Also every snapshot changed slightly because the new snapshots include more info.
So just have a look at a few of these examples in the test folder.

The files in the test_config folder are taken from Prettier’s repo. Only the require_standalone.js and run_spec.js were altered.

require_standalone.js requires the standalone module instead of parsing it and executing it in a separate empty context .
run_spec.js has some variables and checks, that only dealt with specific languages and parsers supported directly by Prettier, removed.

Probably would suggest to only check these 2 files.

Finally there are some changes in the Jest config mainly taken from Prettier’s repo. And a few changes in the package.json scripts.

AST_COMPARE doesn't exist anymore, it was replaced by FULL_TEST which does much more now.
Running the tests won’t collect coverage unless we run them with the FULL_TEST flag. this way we won't get noisy feedback when we run a specific test like npm test tests/Assembly.

Finally you could have a look and play with the result.

mattiaerre · 2021-03-18T00:32:53Z

thanks for the recap @Janther this is great

mattiaerre · 2021-03-23T16:57:09Z

sorry @Janther I was finally able to take a look at this PR, added some comments thanks // cc @fvictorio

mattiaerre · 2021-03-23T16:58:03Z

README.md

@@ -152,8 +152,9 @@ If you want a different configuration for your javascript and solidity files, yo
 1. [Fork it](https://github.com/prettier-solidity/prettier-plugin-solidity/fork)
 2. Create your feature branch (`git checkout -b feature/fooBar`)
 3. Commit your changes (`git commit -am 'Add some fooBar'`)
-4. Push to the branch (`git push origin feature/fooBar`)
-5. Create a new Pull Request
+4. All existing test and coverage must pass (`npm run test:all`), if coverage drops below 100% add missing tests.


not really clear, can you elaborate more on this?

Not sure what else can I write here. Perhaps I'm too close to the forest to see the trees. Maybe you can add something here?

this is my bad sorry, the desc is perfect

mattiaerre · 2021-03-23T17:00:27Z

package.json

    "test": "jest",
-    "test:all": "AST_COMPARE=true npm t -- --coverage"
+    "test:all": "FULL_TEST=1 jest"


can we use true instead of 1? thanks

I used 1 as it is what's used in Prettier's scripts. It can be anything though as the variable is a string and the code just checks for a truthy value. It would also run full tests with FULL_TEST=false jest. Only FULL_TEST= jest will run normal tests as FULL_TEST would be considered a falsy value as it is an empty string, of course no FULL_TEST at all will also be a falsy value since in runtime will be undefined. I'm just pointing out that using true as a value could be misleading since Prettier's test doesn't really parse the flag as JSON and I wouldn't implement that step since I'd like to keep this code as close as possible to the original, that way we can follow Prettier's enhancements to the testing system easier.

alright, I'm ok to keep 1 then thanks for the explanation

mattiaerre · 2021-03-23T17:01:43Z

jest.config.js

 module.exports = {
-  collectCoverage: true,
+  collectCoverage: !!FULL_TEST,


related to my other comment below; not a huge fan of the !! operator

mattiaerre · 2021-03-23T17:05:38Z

tests_config/require_standalone.js

+
+module.exports = {
+  formatWithCursor(input, options) {
+    const $$$options = {


$$$options can you tell me more about it?

Just a name to be different from options it came from the original file. That one instead of importing the code, it reads the files and treats it as a string to be evaluated by the vm. I didn't have much time to go through this process so there's some room for improvement here.

mattiaerre · 2021-03-23T17:08:01Z

tests_config/setup.js

@@ -0,0 +1,7 @@
+const runSpec = require('./run_spec.js');


interesting, it looks like from this file on GH is no longer formatting js? 🤷‍♂️

Looks normal in my screen.

mattiaerre

thanks @Janther good job 🙏

Janther mentioned this pull request Mar 14, 2021

New lines before Assembly labels are not predictable #449

Closed

Janther commented Mar 14, 2021

View reviewed changes

Janther requested review from fvictorio and mattiaerre March 14, 2021 06:09

Janther mentioned this pull request Mar 15, 2021

Eslint and Prettier weren't linting and formatting every file. #447

Merged

Janther force-pushed the tests branch 2 times, most recently from 29acf09 to 44d0e74 Compare March 17, 2021 10:01

mattiaerre requested changes Mar 23, 2021

View reviewed changes

Janther added 10 commits March 24, 2021 15:25

using defineProperty() instead of directly assigning run_spec to global

051b7ae

Using the latest run_spec for testing

fada08a

fix call to run_spec and new snapshots

75b54f9

support for prettier/standalone in tests

3987c4b

running prettier

6cd49d4

only collect coverage in FULL_TEST

dad06e7

cleaning up the scripts in package.json

8b3ccfa

adding testing to the README

0c13456

.eslintignore and .prettierignore take care of the ignored folders now

d97fcbb

Avoid using !!

c9f50a3

Janther force-pushed the tests branch from 44d0e74 to c9f50a3 Compare March 24, 2021 02:32

mattiaerre approved these changes Mar 28, 2021

View reviewed changes

mattiaerre merged commit 7769cd6 into master Mar 28, 2021

mattiaerre deleted the tests branch March 28, 2021 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating Tests to latest Prettier tests #448

Updating Tests to latest Prettier tests #448

Janther commented Mar 14, 2021 •

edited

Loading

codecov bot commented Mar 14, 2021 •

edited

Loading

Janther Mar 14, 2021

Janther Mar 14, 2021

Janther Mar 14, 2021

mattiaerre commented Mar 17, 2021

Janther commented Mar 17, 2021

mattiaerre commented Mar 17, 2021

Janther commented Mar 17, 2021

mattiaerre commented Mar 18, 2021

mattiaerre commented Mar 23, 2021 •

edited

Loading

mattiaerre Mar 23, 2021

Janther Mar 24, 2021

mattiaerre Mar 28, 2021

mattiaerre Mar 23, 2021

Janther Mar 24, 2021 •

edited

Loading

mattiaerre Mar 28, 2021

mattiaerre Mar 23, 2021

Janther Mar 24, 2021

mattiaerre Mar 23, 2021

Janther Mar 24, 2021

mattiaerre Mar 23, 2021

Janther Mar 24, 2021

mattiaerre left a comment

Updating Tests to latest Prettier tests #448

Updating Tests to latest Prettier tests #448

Conversation

Janther commented Mar 14, 2021 • edited Loading

codecov bot commented Mar 14, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattiaerre commented Mar 17, 2021

Janther commented Mar 17, 2021

mattiaerre commented Mar 17, 2021

Janther commented Mar 17, 2021

mattiaerre commented Mar 18, 2021

mattiaerre commented Mar 23, 2021 • 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

Janther Mar 24, 2021 • 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

Choose a reason for hiding this comment

mattiaerre left a comment

Choose a reason for hiding this comment

Janther commented Mar 14, 2021 •

edited

Loading

codecov bot commented Mar 14, 2021 •

edited

Loading

mattiaerre commented Mar 23, 2021 •

edited

Loading

Janther Mar 24, 2021 •

edited

Loading