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

Testing: Remove beautification from block fixtures #8294

Closed
aduth opened this issue Jul 30, 2018 · 7 comments
Closed

Testing: Remove beautification from block fixtures #8294

aduth opened this issue Jul 30, 2018 · 7 comments
Labels
[Feature] Blocks Overall functionality of blocks Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@aduth
Copy link
Member

aduth commented Jul 30, 2018

Related: #7892

As of #7892, we no longer apply HTML beautification to the output of blocks. However, our block test fixtures "original" markup was not updated to reflect this. The original markup should ideally align with more-or-less exactly what is expected as the output of the block itself.

Task: For each .html file (not .serialized.html) in test/integration/full-content/fixtures, remove excess whitespace which would not have been generated in the output of block serialization. This could be done by using the .serialized.html which is expected to be equivalent, but should be done so carefully so as not to simply assume that the .serialized.html is "correct" (as it's the intention of these tests to ensure sensible parse / reserialize flow).

@aduth aduth added [Type] Task Issues or PRs that have been broken down into an individual action to take [Feature] Blocks Overall functionality of blocks Good First Issue An issue that's suitable for someone looking to contribute for the first time [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Jul 30, 2018
@Taym95
Copy link

Taym95 commented Aug 1, 2018

Hi, first of all, thank you for this great editor, I would like to help, can you give an example of which whitespace you mean?

@aduth
Copy link
Member Author

aduth commented Aug 1, 2018

Hi @Taym95 ! Thanks for your willingness to step in. If you look at this folder, you'll see that, for each fixture, there are two HTML files, one ending in .html and another as .serialized.html:

https://github.com/WordPress/gutenberg/tree/master/core-blocks/test/fixtures

The idea with these fixtures tests is that we treat the .html as a copy of the block saved to the database, and want to ensure that the output (.serialized.html) is the same.

For example, consider the gallery fixture:

The first one includes extra tabs and newlines, since this is how prior to #7892 blocks were written to the database. Since this is no longer true, it would be more accurate for us to rewrite them without the excess whitespace.

The process may very well be to copy contents of .serialized.html into the plain .html, for each fixture in this directory. Since tests are currently passing, we should be able to assume that these are still correct / equivalent.

@Taym95
Copy link

Taym95 commented Aug 1, 2018

Hi @aduth , Thank you for clarifications, I got it I will try to submit PR for this issue.

@ecotechie
Copy link

Looks like this is no longer relevant? The files are not there.
Looks like these are them now, but they do look different.

@aduth
Copy link
Member Author

aduth commented Aug 20, 2018

The files have since been moved to:

https://github.com/WordPress/gutenberg/tree/master/test/integration/full-content/fixtures

Looks like these are them now, but they do look different.

As best I can tell, the original point of the task remains (the original .html files include unnecessary/inaccurate beautification).

@gziolo gziolo added the Needs Dev Ready for, and needs developer efforts label Apr 10, 2019
@aduth
Copy link
Member Author

aduth commented Apr 29, 2019

I don't think this is very important to address. In fact, the variations between the source and output could be seen as a good real-world stress-test of whitespace variations.

@aduth aduth closed this as completed Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants