Fix and regenerate invalid block fixtures #2056

aduth · 2017-07-27T17:10:21Z

Related: #1929 (comment)

This pull request seeks to resolve markup errors present in block fixtures to ensure that all fixtures are valid. As described in #1929 (comment), we'll want some intentionally-invalid examples as well, but those are not included in the changes here.

The changes also add some nice conveniences to better compare a failed parse between its actual and expected representation. For example:

This works only in Node environments currently, due to differences in log coloring between Node and browsers. This is something I'd like to consider improving, however separately.

There were a number of failed tests in api/test/parser.js after introducing these changes that I am surprised were ever working, since the dummy block implementations were clearly flawed. Those have been resolved here.

Testing instructions:

Verify that tests pass when regenerating fixtures from scratch:

./node_modules/.bin/rimraf blocks/test/fixtures/*.serialized.html && \
    ./node_modules/.bin/rimraf blocks/test/fixtures/*.json && \
    GENERATE_MISSING_FIXTURES=y npm run test-unit

Ensure there are no regressions with revised pullquote and gallery attribute defaults (moved to default attributes for consistency between edit and save usage).

nylen · 2017-07-30T20:49:22Z

we'll want some intentionally-invalid examples as well, but those are not included in the changes here

I think we might as well handle this in this PR, too: some of the fixtures modified in this PR should serve as our intentionally-invalid examples.

nylen · 2017-07-30T20:55:10Z

blocks/api/parser.js

+			// ANSI codes are supported. In these environments we provide a nicely
+			// colored error message for invalid parse.
+			//
+			// TODO: Teach browsers to ANSI


... or teach chalk to output HTML. ANSI escape sequences are quite convoluted, and it seems like what we really want is an intermediate representation that can be outputted in either form.

... or teach chalk to output HTML. ANSI escape sequences are quite convoluted, and it seems like what we really want is an intermediate representation that can be outputted in either form.

I did run across this: https://www.npmjs.com/package/ansi_up

I'd also toyed with ideas for converting ANSI automatically to CSS-styled console.log depending on the environment.

nylen · 2017-07-30T20:58:10Z

blocks/api/parser.js

+				// eslint-disable-next-line no-console
+				console.warn( message );
+			}
+		}


I think this code block could be structured better to achieve the following goals:

Serve as a foundation for an eventual UI that explains the differences to the user.

Specifically test the logic for generating the difference messages.

We probably do want this code in the production build eventually, so these changes would end up removing most of the environment-specific conditionals. What would remain would be something like this:

if ( 'test' === process.env.NODE_ENV ) { // Maybe we can even remove this, in favor of // `expect( console.warn ).toHaveBeenCalledWith( ... )` // in the tests below. throw new Error( message ); } else if ( 'production' !== process.env.NODE_ENV ) { console.warn( message ); }

The NODE_ENV condition wasn't so much for testing the parser as much as it was to ensure the fixtures testing would actually log the failure if the parse was invalid (console.warn is not sufficient).

Instead, in cdde9c4 I've changed it so that the error is thrown in all cases. It is handled by a try / catch in createBlockWithFallback which assigns the error into the block. With a reintroduction of the invalid blocks, the fixtures testing determines whether the parse should have failed and otherwise surfaces the error to Jest.

I went back and forth on this a bit, trying to decide if throwing an error was the right path. It's otherwise difficult to detect the failure in fixtures testing... except with overriding console.error, which felt a bit hacky. I think they're legitimate errors and should be treated as such.

The downside is that because the error is handled, it is no longer logged to the console in the browser. But because the error is now stored into the block, I think we have some better opportunities to surface the message through the UI, rather than through the console (even in the "Invalid Block" dialog).

To simplify the formatting, I've removed the chalk.isEnabled split and just use chalk in all cases. It degrades gracefully to identity when not enabled.

nylen · 2017-07-30T21:00:10Z

blocks/api/test/parser.js

 			expect( isValidBlock(
 				'Apples',
 				getBlockType( 'core/test-block' ),
 				{ fruit: 'Bananas' }
 			) ).toBe( false );
+
+			process.env.NODE_ENV = env;


I'd rather see this structured as follows:

expect( () => isValidBlock( ... ) ).toThrow( ... );

or, better still:

expect( isvalidBlock( ... ) ).toBe( false ); expect( console.warn ).toHaveBeenCalledWith( ... );

Per above #2056 (comment), this now tests that isValidBlock throws.

aduth · 2017-08-03T02:00:37Z

I think we might as well handle this in this PR, too: some of the fixtures modified in this PR should serve as our intentionally-invalid examples.

The previous invalid versions of the blocks were restored and tested as intentionally invalid as of cdde9c4.

nylen · 2017-08-04T15:19:29Z

blocks/api/test/parser.js

 				'Apples',
 				getBlockType( 'core/test-block' ),
 				{ fruit: 'Bananas' }
-			) ).toBe( false );
+			) ).toThrow();


Should we test a specific error message here, or save this for later when we're ready to expose this info to the UI?

Is a pain to test with the ANSI escapes, unless we were to disable that for testing, or just test the pattern.

when we're ready to expose this info to the UI?

Which I'm starting to wonder, do we want the error message exposed in the UI anywhere? Currently we're only generating and throwing the error in development modes, which means it's not something generally accessible in the UI (in distributed plugin anyways).

To clarify, I think what should be shown in the UI is more like a text-mode diff and a prompt to confirm that the changes are acceptable.

Chalk still works when not enabled, but doesn't add any styles. This was avoided previously because messaging looked poor on per-character diff. We later changed to per-line diff with prefix, which looks reasonable in no-style environments. Further, prefixing retains readability for console.error, so drop thrown error.

nylen · 2017-08-04T15:31:15Z

blocks/test/full-content.js

+			 * Serialize the parsed block objects and verify the output (HTML +
+			 * block delimiters).
+			 */
+
 			// `serialize` doesn't have a trailing newline, but the fixture
 			// files should.
 			const serializedActual = serialize( blocksActual ) + '\n';


Aside: I thought we got rid of this at some point?

nylen · 2017-08-04T15:33:04Z

blocks/test/full-content.js

+			if ( ! isExpectedValid ) {
+				return;
+			}
+


I think for an invalid parse result, we should still verify that the block is initialized and serialized as expected (currently, no change).

aduth · 2017-08-04T15:34:12Z

blocks/test/full-content.js

@@ -95,13 +95,23 @@ describe( 'full post content fixture', () => {

 	fileBasenames.forEach( f => {
 		it( f, () => {
+			/**


We have three styles of multi-line comments:

WordPress standard /*

Calypso standard //

This /**

I think the WordPress standard was written before JSDoc came into wide usage.

I don't really have a preference here, do you? I just wrote them that way so they would visually look like separate sections.

Mine is //, but I also didn't realize 'til recently that this was at odds with the core convention 😬 I'd say I have a preference against this style, only because it is easy to confuse with (and encounter errors as) JSDoc.

I went with /* to preserve the "visual separator" aspect of these comments.

nylen · 2017-08-04T15:34:20Z

blocks/test/full-content.js

 			const content = readFixtureFile( f + '.html' );
 			if ( content === null ) {
 				throw new Error(
 					'Missing fixture file: ' + f + '.html'
 				);
 			}

-			const blocksActual = parse( content );


This declaration should stay below with the rest of the "list of block objects" stuff. I've added comments to better delineate this code into logical steps. In a separate PR, we could try adding new it() test cases for each of tehse steps.

codecov · 2017-08-04T15:39:17Z

Codecov Report

Merging #2056 into master will increase coverage by 0.38%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##           master    #2056      +/-   ##
==========================================
+ Coverage   23.07%   23.46%   +0.38%     
==========================================
  Files         141      141              
  Lines        4403     4424      +21     
  Branches      746      746              
==========================================
+ Hits         1016     1038      +22     
  Misses       2856     2856              
+ Partials      531      530       -1

Impacted Files	Coverage Δ
blocks/library/pullquote/index.js	`33.33% <100%> (ø)`	⬆️
blocks/api/parser.js	`100% <100%> (+2.85%)`	⬆️
blocks/library/gallery/index.js	`26.47% <50%> (+0.75%)`	⬆️
blocks/api/serializer.js	`97.14% <0%> (-2.86%)`	⬇️

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 ae09cbb...269cac5. Read the comment docs.

aduth · 2017-08-07T13:40:40Z

This pull request will need some further thought, particularly around validation errors, which might ought to be split to its own pull request. Notably with #2210, we no longer use beautification to validate post content, so simply logging the "actual" and "expected" contents might not be the most useful to illustrate the true difference that was captured.

A couple considerations:

Beautify contents in logging only
Leverage the fact that we know the precise token where validation failed and highlight it in logged error message. Might be difficult to log in context of the full text, although location can be added into the tokens without much trouble, at which point it might just be some simple slicing.

But as mentioned at #2056 (comment), there's also some open questions about the sorts of errors and messaging we want to be able to display in the UI, not just development-mode console.error.

I might see about piecing apart this pull request into separate ones, as the original goal (title) has certainly drifted.

nylen · 2017-08-07T18:22:16Z

I'd propose that in this PR we keep the fixture file changes and undo the changes to the parser API. I can help out with that over the next couple of days.

aduth added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Jul 27, 2017

aduth mentioned this pull request Jul 28, 2017

Fix fixture serialization with double newline ending #2069

Merged

nylen force-pushed the fix/block-fixtures branch from 8d4942b to 55e85d3 Compare July 30, 2017 20:49

nylen reviewed Jul 30, 2017

View reviewed changes

nylen mentioned this pull request Jul 30, 2017

Set thresholds for codecov build failures #2094

Merged

aduth force-pushed the fix/block-fixtures branch from db23ec0 to cdde9c4 Compare August 3, 2017 01:51

codecov bot deleted a comment from codecov-io Aug 3, 2017

aduth force-pushed the fix/block-fixtures branch 2 times, most recently from 2ab47c7 to 6f5c56b Compare August 3, 2017 16:28

nylen reviewed Aug 4, 2017

View reviewed changes

aduth and others added 6 commits August 4, 2017 11:21

Log parse errors in all environments except production

c8f55a7

Improve error messaging for parse failures

80ca6d0

Fix and regenerate invalid fixtures

eccc524

Fixtures: Add and test intentionally invalid blocks

fefcd7f

Add comments to break up full-content testing into steps

2f5a485

nylen force-pushed the fix/block-fixtures branch from 6f5c56b to 2f5a485 Compare August 4, 2017 15:30

nylen reviewed Aug 4, 2017

View reviewed changes

aduth commented Aug 4, 2017

View reviewed changes

nylen reviewed Aug 4, 2017

View reviewed changes

Tweak comment delimiters

269cac5

nylen force-pushed the fix/block-fixtures branch from 311a2eb to 269cac5 Compare August 4, 2017 15:53

aduth closed this Aug 22, 2017

aduth deleted the fix/block-fixtures branch August 22, 2017 14:26

aduth mentioned this pull request Sep 29, 2017

Validation: Improve developer messaging for invalid blocks #2837

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and regenerate invalid block fixtures #2056

Fix and regenerate invalid block fixtures #2056

aduth commented Jul 27, 2017

nylen commented Jul 30, 2017

nylen Jul 30, 2017

aduth Aug 3, 2017

nylen Jul 30, 2017

aduth Aug 3, 2017

nylen Jul 30, 2017

aduth Aug 3, 2017

aduth commented Aug 3, 2017

nylen Aug 4, 2017

aduth Aug 4, 2017

nylen Aug 4, 2017

nylen Aug 4, 2017

nylen Aug 4, 2017

aduth Aug 4, 2017

nylen Aug 4, 2017

aduth Aug 4, 2017

nylen Aug 4, 2017

nylen Aug 4, 2017

codecov bot commented Aug 4, 2017 •

edited

Loading

aduth commented Aug 7, 2017

nylen commented Aug 7, 2017

Fix and regenerate invalid block fixtures #2056

Fix and regenerate invalid block fixtures #2056

Conversation

aduth commented Jul 27, 2017

nylen commented Jul 30, 2017

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

aduth commented Aug 3, 2017

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

codecov bot commented Aug 4, 2017 • edited Loading

Codecov Report

aduth commented Aug 7, 2017

nylen commented Aug 7, 2017

codecov bot commented Aug 4, 2017 •

edited

Loading