test: custom tests for formatter #2045

ematipico · 2022-02-03T15:00:42Z

Summary

Closes #2044

Updated the test suite of the formatter. Now, it's possible to create test cases with a particular set of options by creating a options.json file beside the test file we want to test.

All files inside a folder that contains options.json will be executed with the set of options written inside the file. Meaning that if you wanted to run only one particular file with some particular options, you would need to put the file inside a separate folder (the PR has an example).

I slightly changed the output of the snapshot, that's why there are lot of changes.

Test Plan

Updated the current snapshots and create a new case with new output and verified that it is written.

cloudflare-workers-and-pages · 2022-02-03T15:00:44Z

Deploying with Cloudflare Pages

Latest commit:	`59945e4`
Status:	⚡️ Build in progress...

View logs

github-actions · 2022-02-03T15:03:13Z

Parser conformance results on ubuntu-latest

T262

Test result	`main` count	This PR count	Difference
Total	45250	45250	0
Passed	44130	44130	0
Failed	1120	1120	0
Panics	0	0	0
Coverage	97.52%	97.52%	0.00%

TS

Test result	`main` count	This PR count	Difference
Total	15976	15976	0
Passed	11213	11213	0
Failed	4762	4762	0
Panics	1	1	0
Coverage	70.19%	70.19%	0.00%

ematipico · 2022-02-03T15:32:27Z

...atter/tests/specs/js/module/expression/import_meta_expression/import_meta_expression.js.snap

 console.log(import.meta);
 import.meta.field =
 	obj.aReallyLongVariableName.andAnotherReallyLongVariableName.andAnotherReallyLongVariableName.andAnotherReallyLongVariable;
 import.meta.aReallyLongVariableName.andAnotherReallyLongVariableName.andAnotherReallyLongVariableName.andAnotherReallyLongVariable;
+## Output 2


This is the new output

crates/rome_formatter/tests/spec_test.rs

crates/rome_formatter/tests/specs/js/module/array/array_nested.js.snap

MichaReiser

Nice! I like the idea and code looks good to me. Let's wait for @xunilrj or @leops to chime in to see if the cfg approach is the right way to go

crates/rome_formatter/tests/spec_test.rs

leops · 2022-02-04T16:27:48Z

crates/rome_formatter/tests/spec_test.rs

+}
+
+#[derive(Debug, Deserialize, Serialize, Clone)]
+pub struct SerializableFormatOptions {


The "remote derive" feature of serde might be useful for this (the facade types don't have to implement From and TestOptions can deserialize to FormatOptions directly): https://serde.rs/remote-derive.html

@ematipico have you tried this?

Yes, and it's not possible with the data structure that I want to have.

Have you seen #2045 (comment)

But agree, this isn't important. I like the name SerializableFormatOptions. It makes it clear what it is about.

crates/rome_formatter/tests/spec_test.rs

CONTRIBUTING.md

ematipico · 2022-02-07T08:46:30Z

I ended up going back to first solution I implemented:

using serde with doesn't work well with vectors, and we would like to keep the possibility to run multiple configurations at once
using features is tricky and some tests might not be run

leops · 2022-02-07T10:31:02Z

Ah yes the #[serde(with = "...")] attribute can't be directly set on a field of type Vec<_> since Vec is its own specific type, instead the elements of the Vec itself would have to to be a newtype like

#[derive(Serialize, Deserialize)]
#[serde(transparent)]
struct SerializableOptions(#[serde(with = "FormatOptionsDef")] FormatOptions);

crates/rome_formatter/tests/spec_test.rs

xunilrj · 2022-02-07T13:40:18Z

Nice! I like the idea and code looks good to me. Let's wait for @xunilrj or @leops to chime in to see if the cfg approach is the right way to go

I would not focus too much on this now, honestly. @leops last suggestion is definitely the best, but I would go with it only on optimizing later.

…ormatter

ematipico · 2022-02-08T12:52:33Z

Other than some optimizations that can be done later on, the PR is ready and it implements what it supposed it.

Can we merge it please?

MichaReiser · 2022-02-08T17:34:17Z

crates/rome_formatter/tests/spec_test.rs

+}
+
+#[derive(Debug, Deserialize, Serialize, Clone)]
+pub struct SerializableFormatOptions {


@ematipico have you tried this?

MichaReiser · 2022-02-08T17:34:55Z

crates/rome_formatter/tests/spec_tests.rs

@@ -4,11 +4,11 @@ mod formatter {

    mod js_module {
        use crate::spec_test;
-        tests_macros::gen_tests! {"tests/specs/js/module/**/**/*.js", spec_test::run, "module"}
+        tests_macros::gen_tests! {"tests/specs/js/module/**/**/**/*.js", spec_test::run, "module"}


This shouldn't be needed. **/*.js should match all JS files regardless of the depth.

MichaReiser · 2022-02-08T17:37:00Z

crates/rome_formatter/tests/spec_test.rs

@@ -22,7 +103,7 @@ use std::path::Path;
 ///
 /// * `json/null` -> input: `tests/specs/json/null.json`, expected output: `tests/specs/json/null.json.snap`
 /// * `null` -> input: `tests/specs/null.json`, expected output: `tests/specs/null.json.snap`
-pub fn run(spec_input_file: &str, _: &str, file_type: &str) {
+pub fn run(spec_input_file: &str, _expected_file: &str, test_directory: &str, file_type: &str) {


Nit: The signature starts to become a bit awkward (so many strings). Could be worth having a SpecTestFile struct that has different attributes like input_file, expected_file, and file_type. I'm not sure what test_directory is, whatever it belongs to the file or is the same for all files.

MichaReiser · 2022-02-08T17:39:01Z

crates/tests_macros/src/lib.rs

+        let mut ancestors = path.ancestors();
+        // the first one will yield the path to the current file
+        ancestors.next();
+        let test_directory = ancestors.next().unwrap().display().to_string();


What exactly is the test_directory. Is it the parent directory of the file? Might be easier to understand if you call path.parent()

ematipico force-pushed the test/custom-tests-formatter branch from b659af6 to 70cbfb3 Compare February 3, 2022 15:00

ematipico force-pushed the test/custom-tests-formatter branch from 70cbfb3 to 066be64 Compare February 3, 2022 15:03

ematipico commented Feb 3, 2022

View reviewed changes

ematipico requested a review from a team February 3, 2022 15:33

MichaReiser reviewed Feb 3, 2022

View reviewed changes

crates/rome_formatter/tests/spec_test.rs Outdated Show resolved Hide resolved

crates/rome_formatter/tests/specs/js/module/array/array_nested.js.snap Outdated Show resolved Hide resolved

MichaReiser requested a review from leops February 4, 2022 13:51

MichaReiser reviewed Feb 4, 2022

View reviewed changes

ematipico requested a review from xunilrj February 4, 2022 13:58

leops reviewed Feb 4, 2022

View reviewed changes

crates/rome_formatter/tests/spec_test.rs Outdated Show resolved Hide resolved

crates/rome_formatter/tests/spec_test.rs Outdated Show resolved Hide resolved

ematipico added 5 commits February 4, 2022 15:54

test: custom tests for formatter

87b7eb1

removed new line

9068e85

applied feature "test" to conditionally apply serde

8a47d5d

change mapping solution for options

c622e8d

rebase

82d6fd4

ematipico force-pushed the test/custom-tests-formatter branch from 6a74933 to 82d6fd4 Compare February 4, 2022 15:56

leops reviewed Feb 4, 2022

View reviewed changes

use remote derive from serde

293ebbc

ematipico force-pushed the test/custom-tests-formatter branch from afffd5c to 293ebbc Compare February 4, 2022 17:15

yassere reviewed Feb 6, 2022

View reviewed changes

crates/rome_formatter/tests/spec_test.rs Outdated Show resolved Hide resolved

crates/rome_formatter/tests/spec_test.rs Outdated Show resolved Hide resolved

crates/rome_formatter/tests/spec_test.rs Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

use conversation to support multiple cases

e07d418

ematipico requested review from yassere, leops and MichaReiser February 7, 2022 08:46

yassere reviewed Feb 7, 2022

View reviewed changes

crates/rome_formatter/tests/spec_test.rs Outdated Show resolved Hide resolved

ematipico force-pushed the test/custom-tests-formatter branch from afa7437 to 1d11040 Compare February 7, 2022 13:17

clippy

0a57548

ematipico force-pushed the test/custom-tests-formatter branch from 1d11040 to 0a57548 Compare February 7, 2022 13:20

ematipico requested a review from yassere February 7, 2022 13:22

ematipico added 3 commits February 8, 2022 11:30

Merge branch 'main' into test/custom-tests-formatter

163d149

fix: merge

a362933

Merge remote-tracking branch 'upstream/main' into test/custom-tests-f…

5e39ee2

…ormatter

MichaReiser removed their request for review February 8, 2022 12:59

MichaReiser approved these changes Feb 8, 2022

View reviewed changes

yassere approved these changes Feb 9, 2022

View reviewed changes

chore: code review

59945e4

ematipico merged commit 1f48995 into main Feb 9, 2022

ematipico deleted the test/custom-tests-formatter branch February 9, 2022 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: custom tests for formatter #2045

test: custom tests for formatter #2045

ematipico commented Feb 3, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 3, 2022 •

edited

Loading

github-actions bot commented Feb 3, 2022 •

edited

Loading

ematipico Feb 3, 2022

MichaReiser left a comment

leops Feb 4, 2022

ematipico Feb 4, 2022

MichaReiser Feb 8, 2022

ematipico Feb 9, 2022

MichaReiser Feb 9, 2022 •

edited

Loading

ematipico commented Feb 7, 2022

leops commented Feb 7, 2022

xunilrj commented Feb 7, 2022

ematipico commented Feb 8, 2022 •

edited

Loading

MichaReiser Feb 8, 2022

MichaReiser Feb 8, 2022

MichaReiser Feb 8, 2022

MichaReiser Feb 8, 2022 •

edited

Loading

test: custom tests for formatter #2045

test: custom tests for formatter #2045

Conversation

ematipico commented Feb 3, 2022 • edited Loading

Summary

Test Plan

cloudflare-workers-and-pages bot commented Feb 3, 2022 • edited Loading

Deploying with Cloudflare Pages

github-actions bot commented Feb 3, 2022 • edited Loading

Parser conformance results on ubuntu-latest

T262

TS

Choose a reason for hiding this comment

MichaReiser 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

MichaReiser Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

ematipico commented Feb 7, 2022

leops commented Feb 7, 2022

xunilrj commented Feb 7, 2022

ematipico commented Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

ematipico commented Feb 3, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 3, 2022 •

edited

Loading

github-actions bot commented Feb 3, 2022 •

edited

Loading

MichaReiser Feb 9, 2022 •

edited

Loading

ematipico commented Feb 8, 2022 •

edited

Loading

MichaReiser Feb 8, 2022 •

edited

Loading