Parser: Runs all parser implementations against the same tests #11320

dmsnell · 2018-10-31T17:08:38Z

So far we haven't added too many tests to the parsers but the ones we
have added are in separate places and each implementation runs against
a different set of tests.

In this patch we're creating shared-tests.js in the spec parser that
defines a suite of conformance tests for the parser and then we're using
that base test-builder to dynamically create test suites for each
implementation such that they all run the same suite.

It's probably easier to understand by reading the code than this
summary.

Of note: by calling to php directly we're able to run the PHP parsers
against the same tests as we are the JavaScript implementations. We
should be able to do this for any implementation as long as the required
binaries are available in the CI environment (Rust, for example).

Update Now this only runs the PHP tests if php is available and running.
The CI environment runs php fine and so does a local build if php is installed.
There's no major need to have the PHP parser tests run on every build though and
in order to not block people who are working on JS these tests will now skip in
such a case.

PHP not available

PHP available

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation. (? maybe)

aduth

Would be good if we can be tolerant to environments where PHP is not available, though not strictly a blocker.

aduth · 2018-10-31T18:08:06Z

packages/block-serialization-default-parser/test/index.js

-	} );
-} );
+describe( 'block-serialization-default-parser-js', testParser( parse ) );
+describe( 'block-serialization-default-parser-php', testParser( ( document ) => JSON.parse(


I expect this may be shadowing the global DOM document.

it will be. I forgot we are avoiding that…will change

dmsnell · 2018-10-31T18:59:03Z

Mobile tests are failing because of an error with the deprecated setUnknownTypeHandlerName() function and they don't appear to be related to this PR. I'll go ahead and merge with those failing tests as long as the other tests pass.

Tests are running all clear locally for me.

Rebased against master where the broken tests were removed

So far we haven't added too many tests to the parsers but the ones we _have_ added are in separate places and each implementation runs against a different set of tests. In this patch we're creating `shared-tests.js` in the spec parser that defines a suite of conformance tests for the parser and then we're using that base test-builder to dynamically create test suites for each implementation such that they all run the same suite. It's probably easier to understand by reading the code than this summary. Of note: by calling to `php` directly we're able to run the PHP parsers against the same tests as we are the JavaScript implementations. We should be able to do this for any implementation as long as the required binaries are available in the CI environment (Rust, for example).

gziolo · 2018-10-31T19:21:00Z

packages/block-serialization-spec-parser/test/__snapshots__/index.js.snap

+exports[`block-serialization-spec-parser-php basic parsing parse() works properly 1`] = `
+Array [
+  Object {
+    "attrs": Array [],


This is concerning that it produces different type for attrs property.

Should we add test which validates that both outputs generated by parsers are deeply equal?

probably something to look into yes, but not here?
I don't think this patch introduced the change so much as brought it to light.

I'm just leaving a note that it doesn't work as expected. The second parser doesn't have this issue :)

gziolo · 2018-10-31T19:27:56Z

packages/block-serialization-default-parser/test/index.js

 /**
 * Internal dependencies
 */
+import { jsTester, phpTester } from '../../block-serialization-spec-parser/shared-tests';


We should have some place to keep shared logic :)
With the current setup of repository and Lerna flow for publishing, this folder will get published to npm. It's not a big deal. However, we should figure out how to tackle it. Maybe it's okey to keep it in the spec parser package but we should import it as:

import { jsTester, phpTester } from '@wordpress/block-serialization-spec-parser/shared-tests';

and include @wordpress/block-serialization-spec-parser as a dev dependency?

I changed to use this and then moved jsTester and phpTester into index.js. this created a new problem which was the import of node modules in the code.

to work around this I hid require( 'child_process' ) behind a 'test' === process.env.NODE_ENV check

do you think this is a reasonable mechanism to keep the unwanted node code out of the runtime while preserving it in the tests?

gziolo

Nice, I like it. I got scared at first seeing PHP file included, but it makes perfect sense to consolidate it here to be able to share tests. Awesome idea 👍

…rnmobile/port-quote-block-step-1 * 'master' of https://github.com/WordPress/gutenberg: (22 commits) Add removed periods to block descriptions. (#11367) Remove findDOMNode usage from the inserter (#11363) Remove deprecated componentWillReceiveProps from TinyMCE component (#11368) Create file blocks when dropping multiple files at once (#11297) Try avoiding the deprecated findDOMNode API from DropZone Provider (#11168) Fix: make meta+A behaviour of selecting all blocks work on safari and firefox. (#8180) Remove _wpGutenbergCodeEditorSettings and wp.codeEditor assets (#11342) Remove the Cloudflare warning (#11350) Image Block: Use source_url for media file link (#11254) Enhance styling of nextpage block using the Hr element (#11354) Embed block refactor and tidy (#10958) Nonce Middleware: Wrap the nonce middleware function into it's own function that isn't regenerated on every API request. (#11347) Fix RTL block alignments (#11293) RichText: fix buggy enter/delete behaviour (#11287) Remove code coverage setup (#11198) Parser: Runs all parser implementations against the same tests (#11320) Stop trying to autosave when title and classic block content both are empty. (#10404) Fix "Mac OS" typo + use fancy quotes consistently (#11310) Update documentation link paths (#11324) Editor: Reshape blocks state under own key (#11315) ... # Conflicts: # gutenberg-mobile

In #11320 we started running all parsers against the same set of unit tests. Unfortunately when the PHP-based parsers failed inside the PHP process or returned non-JSON data then the tests would fail without providing any information about why. My personal workaround was to manually run the PHP test runner from the command line to see the output. This was inefficient. In this patch we're trapping the response code from the PHP test runner and throwing an `Error` with the output from `php` so that our `jest` tests can see and report them. This will make it easier debug failing tests.

dmsnell added [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Oct 31, 2018

dmsnell requested review from mcsf, pento, gziolo and aduth October 31, 2018 17:08

dmsnell mentioned this pull request Oct 31, 2018

WordPress 5.0 beta1 - Block now causing Fatal Errors #11066

Closed

aduth approved these changes Oct 31, 2018

View reviewed changes

dmsnell added 6 commits October 31, 2018 15:05

update snapshot

922867d

add explicit timeouts to spanSync call

42adae3

skip tests if php is unavailable

d066372

skip tests if php is unavailable

becc7dd

unshadow document

2c1d61d

dmsnell force-pushed the parser/unify-tests branch from 749b9c7 to 2c1d61d Compare October 31, 2018 19:05

gziolo reviewed Oct 31, 2018

View reviewed changes

gziolo added this to the 4.3 milestone Oct 31, 2018

gziolo approved these changes Oct 31, 2018

View reviewed changes

dmsnell added 3 commits October 31, 2018 16:29

use package for test runner import

1a09538

conditionally compile test code?

0203593

use file path for npm dependency

f5f501b

dmsnell merged commit b7d8fbf into master Oct 31, 2018

dmsnell deleted the parser/unify-tests branch October 31, 2018 21:12

dmsnell mentioned this pull request Nov 2, 2018

Parser: Pass-through PHP errors in tests #11429

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parser: Runs all parser implementations against the same tests #11320

Parser: Runs all parser implementations against the same tests #11320

dmsnell commented Oct 31, 2018 •

edited

Loading

aduth left a comment

aduth Oct 31, 2018

dmsnell Oct 31, 2018

dmsnell commented Oct 31, 2018 •

edited

Loading

gziolo Oct 31, 2018

dmsnell Oct 31, 2018

gziolo Oct 31, 2018

gziolo Oct 31, 2018

dmsnell Oct 31, 2018

gziolo left a comment

Parser: Runs all parser implementations against the same tests #11320

Parser: Runs all parser implementations against the same tests #11320

Conversation

dmsnell commented Oct 31, 2018 • edited Loading

Checklist:

aduth left a comment

Choose a reason for hiding this comment

aduth Oct 31, 2018

Choose a reason for hiding this comment

dmsnell Oct 31, 2018

Choose a reason for hiding this comment

dmsnell commented Oct 31, 2018 • edited Loading

gziolo Oct 31, 2018

Choose a reason for hiding this comment

dmsnell Oct 31, 2018

Choose a reason for hiding this comment

gziolo Oct 31, 2018

Choose a reason for hiding this comment

gziolo Oct 31, 2018

Choose a reason for hiding this comment

dmsnell Oct 31, 2018

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

dmsnell commented Oct 31, 2018 •

edited

Loading

dmsnell commented Oct 31, 2018 •

edited

Loading