Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validation of .json fixtures causes bad formatting to be applied #884

Closed
Squiggle opened this issue Nov 7, 2017 · 9 comments
Closed

Validation of .json fixtures causes bad formatting to be applied #884

Squiggle opened this issue Nov 7, 2017 · 9 comments
Labels
type: unexpected behavior User expected result, but got another
Milestone

Comments

@Squiggle
Copy link

Squiggle commented Nov 7, 2017

  • Operating System: Windows 10
  • Cypress Version: 1.0.2
  • Browser Version: n/a

Is this a Feature or Bug?

Bug.

Current behavior:

Fixture validation causes bad formatting for .json files, resulting in too many line breaks and no indentation.

e.g.

{
  
"modificationDate": "2017-10-27T14:40:27Z",
  
"isActive": false,
  
"contents": {
    
"en": {

For large fixtures it makes it difficult to read and edit.

Desired behavior:

Formatting for .json files should be indented, e.g.

{
  "modificationDate": "2017-10-27T14:40:27Z",
  "isActive": false,
  "contents": {
    "en": {

How to reproduce:

Running tests, reference a .json fixture. It will automatically format it.

Test code:

n/a

Additional Info (images, stack traces, etc)

n/a

@brian-mann
Copy link
Member

We use jsonlint under the hood to do this.

https://github.com/cypress-io/cypress/blob/preprocessor-refactor/packages/server/lib/fixture.coffee#L10

We have a lot of tests around formatting as well. I'm unsure how or what is causing the line breaks but I don't see this happening in our own fixtures. It might be triggered by something else that's special about your fixtures.

We'll need more information from you in order to investigate.

@brian-mann brian-mann added the stage: needs information Not enough info to reproduce the issue label Nov 7, 2017
@MarcLoupias
Copy link

Got the same issue here on Windows 7. No problem on linux OS.

The formatter rewrite the fixtures files (don't know why it actually do any I/O on them) and insert innaproriate CR chars.

To reproduce simply run a project test suite using fixtures on a Windows. The run will changes the fixtures files.

@brian-mann
Copy link
Member

I see. Okay so it's a windows problem then. Thank you.

@brian-mann brian-mann added OS: windows type: unexpected behavior User expected result, but got another and removed stage: needs information Not enough info to reproduce the issue labels Nov 7, 2017
@MarcLoupias
Copy link

Notice the issue is not really with EOL chars but with the fixtures files rewrite when running cypress.

You could have source code with LF EOL set in the .editorconfig file with a windows working station.

The formater should not write into the fixtures files, that's the problem here.

@MarcLoupias
Copy link

MarcLoupias commented Nov 16, 2017

Reading the gitter i have seen the discussion between @egucciar and @brian-mann about this.

I am also very concerned about this.

I don't think cypress should write any fixture file at all, and in fact the default behavior should be read-only.

Writing fixtures files with a linting / formating process should be an extra optionnal feature.

Parsing the files add value because it enable a validity check before the test suite run so there is things to keep here but cypress should not write anything by default.

Rewriting files forces cypress to infer about project practices. Is it tab spaced ? 2 spaces ? 4 spaces ? Is EOL CRLF ? LF ? What is the criteria to define EOL, is it git ? Is it .editorconfig file ? Is it OS ? Or something else i don't know ?

The fact is you could have 3 devs on windows, 2 on MacOS and 1 on Linux, and EOL could be defined in git config or in .editorconfig in the project file, etc ...

The fixtures files are part of the project owner and should rely on the project owner config, not on the cypress default config.

The last days i have put some time reading fixture management source code in cypress, i would like to try a super simple PR :

  • adding a readOnly option in the fixture config object. If true the writeFileAsync is not triggered.
  • adding a global config (like the timeout one) to makes the readOnly effective everywhere without the need to change it on each fixture.

This could be a quick win without all the refactoring and behavior changes that needs time to be discussed.

If you agree with the principle i could push my PR around friday night EU TZ with the documentations changes.

@jennifer-shehane jennifer-shehane added stage: proposal 💡 No work has been done of this issue type: unexpected behavior User expected result, but got another and removed type: unexpected behavior User expected result, but got another OS: windows labels Nov 16, 2017
@brian-mann
Copy link
Member

I am going to fix the issues with Windows and then add a configuration option to turn this off.

We've had thousands of users go through Cypress and only a handful have complained about fixture formatting. I'd like to leave that as the default and give people the option to opt out.

@brian-mann
Copy link
Member

EDIT:

Ok fine.

We're going to turn this off by default but enable an option to turn this back on.

I am going to fix the issues with Windows formatting.

@brian-mann
Copy link
Member

@bahmutov feels strongly that we should remove this feature, so we are.

@brian-mann brian-mann added this to the 1.1.0 milestone Nov 20, 2017
@brian-mann
Copy link
Member

Fixed in 1.1.0.

@jennifer-shehane jennifer-shehane removed the stage: proposal 💡 No work has been done of this issue label Nov 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: unexpected behavior User expected result, but got another
Projects
None yet
Development

No branches or pull requests

4 participants