[regression] Restore \LocaleJavascriptTranslationTest test coverage and keep testing processed JS file

@droplet

Great. Since this this now blocking #2880007: Auto-fix ESLint errors and warnings, and this is a pretty simple fix. 🚀👍

+++ b/core/modules/locale/tests/locale_test.js.fake
@@ -2,7 +2,10 @@
+ * The Locate module parses the strings in JavaScript files with simple regex.
+ * We keep this non-ES6 original file for testing purpose. This is no ES6
+ * version available.

Let's review the wording here. Eg. it is definitely not the Locate module :), etc.

- fix comment.

- for testing purposes

good to go.

+++ b/core/modules/locale/tests/locale_test.js.fake
@@ -2,7 +2,10 @@
+ * @preserve
+ * The Locale module parses the strings in JavaScript files with a simple regex.
+ * We need to keep this non-ES6 original file for testing purposes. There is no
+ * ES6 version available.

Thanks for fixing these. Now that the comment is clear, let me ask why it was a problem that the file got processed. If the ES6 processing breaks the source string parsing that Locale module does, than why would it not break for all the other JS files in core?

Drupal parses the processed result at runtime doesn't it?

Discussed the issue with @drpal in chat. Posting for transparency. I believe we would need to parse the file as Drupal will encounter it, otherwise we are not really testing that Drupal is capable of extracting translatable strings from its JS and consequently that it can translate JS strings, but we are just testing that some JS file could be parsed.

gaborhojtsy [2:59 PM]
@drpal one more question on https://www.drupal.org/node/2889600 to make sure I understand it :)

drpal [4:07 PM]
@gaborhojtsy Overall, I think we’re doing the wrong thing with the tests, but that’s not really important here.
@gaborhojtsy After we copied the original JS file to the ES6 extension and then transpiled back, Babel got rid of the new line characters and some of the white space for the test data.
Which we are unfortunately depending on in testing.

gaborhojtsy [4:08 PM]
@drpal so we should fix the test then, otherwise it looks like the transpiled JS will not be localizable as such?
@drpal trying to mask that with a test workaround does not sound good

drpal [4:08 PM]
¯\_(ツ)_/¯

gaborhojtsy [4:08 PM]
@drpal we are meant to test the JS that Drupal will encounter live

drpal [4:09 PM]
I can only fix so much at once.
This is blocking the larger es6 auto fix patch.

gaborhojtsy [4:09 PM]
@drpal do we have an issue about JS translations not working anymore then?
drpal [4:09 PM]
Anything inside `Drupal.t()`
Should still work just fine.

gaborhojtsy [4:11 PM]
@drpal so can we make sure the test ensures the transpiled file works?

drpal [4:12 PM]
@gaborhojtsy The patch from here, https://www.drupal.org/node/2880007#comment-12121300 actually fixed the tests.
And then see droplet’s comment after, https://www.drupal.org/node/2880007#comment-12121442

gaborhojtsy [4:16 PM]
@drpal I read the comments, its not clear to me what is going on here sorry
@drpal if babel is fixing a problem that we are testing for, then if all JS is run through that chain then should we be testing for that problem anymore?
@drpal if contrib would still not do it yet, then we can have a file that is runthrough babel to ensure that the pipline fully works and split the file to another one with just the “problematic” ones so THOSE are not fixed by babel

drpal [4:30 PM]
Alright, coffee in hand.
> if babel is fixing a problem that we are testing for
Babel isn’t really fix anything, it can’t, its just a syntax transpiler.
However, we expect the format of some strings to be passed to the test in a specific format, Babel doesn’t know anything about that, so it does what it knows about.
There’s also something, `SafeMarkup::format()`, I don’t know anything about PHP but whatever is happening there looks like it’s comparing two strings.
So we’re comparing a string in PHP with a string defined in JavaScript. (edited)
If we want to make a case for testing the babel-processed js, that’s fine, I honestly don’t care.
But I really don’t want to see us investing a bunch of effort in our existing javascript testing infrastructure when it’s already mostly unsupported.

gaborhojtsy [4:51 PM]
@drpal so my point of view is we have translatable strings in JS, we have tests to ensure that Drupal can parse those strings and yes it does have expectations of the format of those JS expressions because the way we translate strings in JS is we also add a string mapping JS file *with the strings needed for the JS files in the request* and the only way to tell the list of strings is if we parse the JS (in PHP), so we know which translations to pull from the db and generate into that JS file
@drpal if we are not testing the *final* JS files that Drupal will encounter because they don’t adhere to the format Drupal’s PHP expects. that means string translation is broken and not that we should work around the problem in the test by testing the JS file that Drupal will not encounter (but is still correct)

drpal [4:55 PM]
I don’t think this has much to do with string translation being broken.
It’s just the test data.

gaborhojtsy [4:56 PM]
@drpal can someone who encounters the broken test data post the breaking test data there then?

drpal [4:57 PM]
What?

gaborhojtsy [4:57 PM]
@drpal if it would be t(‘^&boo’) it would not break but because its t(‘!$5boo’) it does or something
@drpal so far we know its *some* problem wiht the test data
or maybe I missed where the offending test data segment/sample/line was pointed out?
it could very well be that I missing something here

drpal [4:59 PM]
https://www.drupal.org/pift-ci-job/690763
Did you see the failed test?

gaborhojtsy [5:00 PM]
yes
how do I interpret this? the transpiler changes the double quoted string with an escaped doouble quote to a single quote?

drpal [5:01 PM]
Yes.

gaborhojtsy [5:02 PM]
ok I did not see that explained before on the issues
so in the test data we are testing that the parser finds escaped single quotes and escaped double quotes
Drupal.t(‘Single Quote \‘Escaped\’ t’);
Drupal.t(‘Single Quote ’ + ‘Concat ’ + ‘strings ’ + ‘t’);
Drupal.t(“Double Quote t”);
Drupal.t(“Double Quote \“Escaped\” t”);
Drupal.t(“Double Quote ” + “Concat ” + “strings ” + “t”);
but babel would only break the escaped double quote
“break” as in not allowed to happen in the transpiled file

drpal [5:04 PM]
I think that’s correct.

gaborhojtsy [5:04 PM]
would it still appear in the transpiled file if there is ALSO a single quote in the string?

drpal [5:05 PM]
I don’t have 100% insight into the exact issue between our test data and whatever php is expecting.
Since that process is totally not clear.

gaborhojtsy [5:05 PM]
we are making sure in the test that the PHP regex can ignore escaped single and double quotes which is important to test for
unless there is no way anymore that an escaped double quote can appear in a JS string
which is why I said would it still appear if we ALSO have a single quote in the string?

drpal [5:06 PM]
Yeah. Ok. Then I have no idea.
I give up. (edited)

gaborhojtsy [5:07 PM]
I think these are the kind of discussions we should have on the issue, because removing a step from the test to ensure it still passes does not ensure that the PHP will still parse what Drupal gets because Drupal gets something else

@droplet: why is it failing on the bot then?

So looks like we don't force a tool but we did make a choice of a tool on the testing server which is why it failed? Is that a fair understanding?

What would this tool or other tools do that would fail our JS parsing? While @drpal said it is futile to add more effort to our existing JS testing infrastructure when we are about to drop it, what happens here is we *need to parse JS in PHP* so we'll need to keep testing that parsing in PHP regardless of what happens to the testing of the JS itself. Because we need to test our PHP code in terms of how it behaves with our JS code. Looks like now with the JS tools where we don't force the hands of our devs the variety of what could happen to our JS grew which is why the test failed.

Do we have a copy of what babel (or whatever the tool used on testbot) did/does to the file that broke the tests? Ie. how does the JS look like that failed? (not from that specific fail, but as reproducible locally)?

Do we have a list of "mostly used JS processors" that can run on the sample JS file and see if they make changes to the file that would be incompatible with what we parse in PHP? This last thing I would not hold against this patch because its a bit open ended to be honest. But for the previous point it would be important to ensure that at least the processor that our testing infrastructure runs with produces files that are compatible with the PHP parser in Drupal that parses the JS at runtime.

Ok looked at the issue summary again. It says:

The ES6 builder (Babel) removed some test cases from this file.

Comparing the two files, it looks like these were transformed to be on one line each:

Drupal
  .
  t
  (
    "Whitespace Call t"
  )
;

Drupal
  .
  formatPlural
  (
    1,
    "Whitespace Call plural",
    "Whitespace Call @count plural"
  )
;

Otherwise test cases were not removed were they? The quoted quotes, etc. are still the same. The test fails @drpal pointed me to are failing on the lack of quoted quotes.

Also the issue summary says

Change the file extension name to .fake to bypass the Babel transforms.

but it does not seem to currently have anything around the quote problems that @drpal pointed to as being failures on testbot. So what is being run on the file additionally in the test environment that transforms the quoted quotes and makes it fail?

Will Drupal encounter the eslinted files though at runtime or will it encounter the untouched files as this test wants to make it appear like? If it will encounter the eslinted files, then we should have a way to test that the PHP code that Drupal runs works fine with those eslinted files. Otherwise we are testing Drupal's PHP parsing JS that is not really Drupal's JS.

Where will that eslint run? On commit? On Drupal sites? Will it run for contrib modules? As per #2880007: Auto-fix ESLint errors and warnings it seems like this is a one-time manual run. Can we update the testing JS in this issue to keep the escaping intact. Eg. add a single quote as well to the double quoted escaped string to make it not transform to a single quote wrapped string? Ie.

-Drupal.t("Double Quote \"Escaped\" t");
+Drupal.t("Double ' Quote \"Escaped\" t");

Ok I tried keeping myself from submitting a patch here because that now makes me ineligible to commit this. But since there does not seem to be another way to move this forward there you go.

Drupal works fine with those eslinted files.

Once again I am not claiming it would not work fine with those files now. What I am saying is we should let the tested JS file go through the same linting / optimization / build steps whatever there going to be (whatever the name of the tool and regardless of whether it runs on the client or server or testbot or in the build process or on github or whatever). Because that is the JS file that Drupal will ultimately see when it parses the JS file for source strings.

The test is ensuring that what JS file Drupal encounters will be possible to parse with the regex in the PHP code so the translations can be pulled and put into the response. If we remove this test JS file from the process pipelines because the pipeline modifies it in a way that is inappropriate, we are creating a dead end where the file will not be picked up by any of the build tools, linters, whatevers and we would never know if the actual final JS files that Drupal encounters will be possible to parse with our PHP code. We will only know the JS file that we purposefully created and fixed in place (before build tools and linters were on the table) can be parsed by our PHP. That is of little assurance that Drupal can also parse the live JS it encounters. These are two different things.

That is why I am once again advocating to find ways to make this JS file keep (some of) the characteristics we are testing for. Can we at least explore this direction before we discard it?

What would the eslint patch do to JS modified like this? (Only setting to needs review for testbot, not claiming this is ready).

Post phpunit conversion variant so it applies.

You have to understand what \Drupal\Tests\locale\Functional\LocaleJavascriptTranslationTest does at first place.

@droplet: As far as I see it checks that the Drupal side PHP can parse the JS file with various syntaxes and it finds the translation source strings regardless of what syntax quirks are in the JS file (eg. escaped quotes, whitespace, concatenation, etc). What am I missing?

This issue is blocking #2880007: Auto-fix ESLint errors and warnings, and is potentially blocked by #2892150: Decide if we parse wired code style syntax in Locale JS strings.

I think Gábor Hojtsy & droplet were mostly talking past each other. Here's my feedback.

I'd RTBC this issue, but I first want to see that they both +1 what I'm saying.

The ES6 builder (Babel) removed some test cases from this file.

Doesn't that mean the test coverage itself was not strict enough?

The point is we made a STRICT test to Loose test.

Indeed. By renaming locale_test.js to locale_test.es6.js and compiling it to locale_test.js in #2818825: Rename all JS files to *.es6.js and compile them , we made the test a loose one, since Babel did some processing on the various Drupal.t() calls and Drupal.formatPlural() calls … and those calls are the edge cases that were being tested in \Drupal\Tests\locale\Functional\LocaleJavascriptTranslationTest::testFileParsing().
Further indication that this happened by accident: core/.eslintignore lists modules/locale/tests/locale_test.js explicitly as one of the files to ignore!

This is exactly what the patch in #8 does. Although I think it'd be better to not name it *.js.fake, but just *.js. That then actually reflects reality for e.g. contrib & custom code. Can we make Babel skip this file some other way? According to https://babeljs.io/docs/usage/babelrc/ we could do something like:

diff --git a/core/package.json b/core/package.json
index 6e9a49c..6cd99c7 100644
--- a/core/package.json
+++ b/core/package.json
@@ -32,6 +32,9 @@
     "stylelint-no-browser-hacks": "^1.0.2"
   },
   "babel": {
+    "ignore": [
+      "modules/locale/tests/locale_test.js",
+    ],
     "presets": [
       [
         "env",

Now that the comment is clear, let me ask why it was a problem that the file got processed. If the ES6 processing breaks the source string parsing that Locale module does, than why would it not break for all the other JS files in core?

It wasn't a "problem" that the file was processed. The only problem here is that the effective test coverage of \Drupal\Tests\locale\Functional\LocaleJavascriptTranslationTest::testFileParsing() was undermined: it was no longer testing many edge cases, but only a few, because Babel's processing simplified many of the special cases, to a degree where they weren't edge cases anymore, but common cases, which of course defeats the purpose of \Drupal\Tests\locale\Functional\LocaleJavascriptTranslationTest::testFileParsing() giving us the assurance that even edge cases were being parsed correctly by our PHP code that is parsing translatable strings from JS. That's all!

What I am saying is we should let the tested JS file go through the same linting / optimization / build steps whatever there going to be (whatever the name of the tool and regardless of whether it runs on the client or server or testbot or in the build process or on github or whatever). Because that is the JS file that Drupal will ultimately see when it parses the JS file for source strings.

Except that's not really true. Contrib/custom modules that don't use .es6.js files, but just .js files, will not get these transformations (transpilations) applied.

I do understand that you also want to test the results of those transpiled files. But those transpiled files use a strict subset of the crazy edge cases you could construct manually, which is exactly what locale_test.js in HEAD shows: because it removes some "craziness", brings more uniformity, we lost test coverage. That's all there is.

Contrib/custom modules that don't use .es6.js files, but just .js files, will not get these transformations (transpilations) applied.

As of right now, our transformations are specific to what's in the core/ directory.

I do understand that you also want to test the results of those transpiled files.

I think actually makes sense to not test the results from Babel. If we are always going to be second-guessing the output from the transpiler, we should probably have just not done this process at all. I do think we should actually be testing the .es6.js files.

I think it'd be better to not name it *.js.fake, but just *.js.

This is probably a better idea.

Overall, ➕1️⃣ on RTBC.

Given that #2880007: Auto-fix ESLint errors and warnings was just committed, I think its hard to argue this is blocking #2880007: Auto-fix ESLint errors and warnings :)

I do understand that you also want to test the results of those transpiled files. But those transpiled files use a strict subset of the crazy edge cases you could construct manually, which is exactly what locale_test.js in HEAD shows: because it removes some "craziness", brings more uniformity, we lost test coverage. That's all there is.

All I am arguing in this issue since day 1 is how do we know that there are not NEW syntaxes introduced by transpilers that core is not testing against? Did we check that? For all the transpilers we support? A sure-fire way to ensure that is done is to test the transpiled files. Or at least have a file that is tested transpiled and one that is tested verbatim (to ensure that quirks removed by the transpiler are tested). But also ensure that quirks possibly ADDED by the transpiler are also covered.

And when I wrote transpiler understand it as "whatever changes the JS code from what it was", as in the figure in #33.

how do we know that there are not NEW syntaxes introduced by transpilers that core is not testing against?

I think all the permutations in locale_test.js are pretty much all the possible permutations already, including the ones that transpilers might create. I think this is why nobody thought of adding explicit additional test coverage. This is also why I'm arguing that just keeping the existing locale_test.js as it was before the ES6 conversion should be enough.

I think we can proceed here?

Per @xjm, promoting to major.

Gábor's concern is that if we have transpilation, are we

1. losing test coverage if we also transpile locale_test.es6.js?
2. how do we know that any possible transpilation is still correctly parsed by the Locale module?
3. how do we know that transpilers are introducing new syntaxes?

i.e.:

1. Point 1 is only about the particular test coverage that we have, and the fact that we transpiled it and this issue is proposing to change it.
2. Point 2 is asking a broad question about all possible edge cases that transpilation introduced and the effect of that on Locale.
3. Point 3 is asking how we can protect ourselves against future evolution in the transpiler.

The answers:

1. Yes, transpiling locale_test.es6.js made a strict test a loose test, because the original file (locale_test.es6.js) has many more edge cases, the transpiled file (locale_test.js) is normalized. locale_test.es6.js still contains all the exotic cases, but rather than testing that, we're testing locale_test.js, which has far fewer exotic cases (because they've been normalized away) This is the regression that was introduced in #2818825: Rename all JS files to *.es6.js and compile them .
2. We could also test the transpiled JS (so locale_test.js), but it would be a subset of the syntaxes used by locale_test.es6.js, so it'd be kind of pointless to test this.
3. The only way new syntaxes could be introduced is if we change the target language: as long as it's ECMAScript 5, no new syntaxes can be introduced.

The solution is therefore to either:

• A) undo the rename of locale_test.js to locale_test.es6.js, and not have a transpiled file. This is what #8 does.
• B) or keep it as-is, but update LocaleJavascriptTranslationTest to test both locale_test.es6.js and locale_test.js, even though the second file will be a subset of the former and therefore be a kind of pointless test. This is what #44 does.

I think #8 makes most sense. So reuploading that file and RTBC'ing.

We could also test the transpiled JS (so locale_test.js), but it would be a subset of the syntaxes used by locale_test.es6.js, so it'd be kind of pointless to test this.

Not true. HEAD's locale_test.js contains { context: (space after brace), whereas locale_test.es6.js contains {context: (no space after brace). Potentially the best thing to do would be to add all the permutations of possible spacings to locale_test.es6.js, but it would be really hard to ensure that we've really caught all possible permutations. Therefore, I think testing the transpiled version in addition to the ES6 version is helpful as an extra double check.

Therefore, of all the patches on this issue, I like #44 the most. Here's just an addition of a code comment explaining to future readers why we're testing both files.

Alright. I think that #51 is most appropriate resolution. Testing both variations gives us good coverage for most of the variations. 👍🍕

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!