Comments

droplet created an issue. See original summary.

droplet’s picture

Or change the file ext? Will it work?

drpal’s picture

Status: Needs review » Reviewed & tested by the community

@droplet

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

droplet’s picture

Issue summary: View changes
droplet’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

drpal’s picture

Status: Needs work » Needs review
FileSize
3.54 KB
685 bytes

- fix comment.

drpal’s picture

- for testing purposes

nod_’s picture

Status: Needs review » Reviewed & tested by the community

good to go.

Gábor Hojtsy’s picture

+++ 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?

droplet’s picture

BabelJS breaks the TESTs (data) we setup, not the parser. I thought the test data should be always in plain format, no matter in whatever languages. (unless we're going to test the processed result)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

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

droplet’s picture

Nope, I don't think babel is a must in all drupal development. On another hand, Minifier may change the code syntax also. We parsing all valid Javascript syntax, so that we also need to test all possible variants to ensure we do not miss anyone.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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

droplet’s picture

I scan the suuuuuuper long chat quickly.

Basically, you should forget all ES6, Babel, other issues bot failing first. With or without those things don't matter.

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

We do not forced all Drupaler to use babel in their develoment. This is very important, we never announce it and I think we will not do it also. Typescript is also a super cool tool :p. And we able to use min.js in Contrib also. I don't know and will not predict how the JS file looks like.

What we should care:
1. The parser able to parse all strings in valid Javascript code
2. The tests able to catch all possible cases in #1.

So, give you an example.

B1. I code my module's JS this way:
`
Drupal
.
t
(
"Whitespace Call t"
)
;
`

B2. I submit a patch to refactor the locale parser and accepted because the bot is GREEN. In my patch I will remove the regex to capture B1 syntax.

B3. my module doesn't work.

Is it a bug?

Let's me know if I need to explain it further more :) (I'm sure I can't explain it any better way. hehe)

droplet’s picture

If you think,... OH Babel may get things more UGLY than we predicted, then we must add an extra test to test babel processed code also.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

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.

droplet’s picture

Issue summary: View changes

So looks like we don't force a tool

Correct.

make a choice of a tool on the testing server which is why it failed? Is that a fair understanding?

Ahh I think you misunderstood @drpal's word. The failture on #2880007: Auto-fix ESLint errors and warnings is caused by ESLint.

The patch in this issue is to restore the test cases.

This is 2 different issues. Please @see IS.

Will it make more sense?

The ES6 builder script in CORE transpiles all Javascript files ending with .es6.js into different code style. This step will simplify the CORE test cases we defined. To avoid this happening, we use .fake as file extension to bypass the ES6 builder script

Personally, I perferred the first patch I've uploaded. The seoncd patch just making the commiters happier and less confuse in the future: https://www.drupal.org/node/2880007#comment-12143062

This is why `.js.fake`, not `.js`

Gábor Hojtsy’s picture

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?

droplet’s picture

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?

Well.. Both strings in JS and Test's PHP should be identical. The ESLint remove the "escape" in SOURCE FILE, that's why the test is failed.

This is how ESLint autofix does:

Drupal.formatPlural(1, 'Double Quote "Escaped" plural', 'Double Quote "Escaped" @count plural');

This is orginal code and defined in TESTS

Drupal.formatPlural(1, "Double Quote \"Escaped\" plural", "Double Quote \"Escaped\" @count plural");

You should count the failtures tests on #2880007: Auto-fix ESLint errors and warnings is a INCORRECT PATCH. We DID NOT find any new bug around the test case. No additional tests will be requried.

droplet’s picture

Title: Restore locale_test.js » [regression] Restore locale_test.js
Related issues: +#2818825: Rename all JS files to *.es6.js and compile them
droplet’s picture

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

This step is prevent a script to reduce our tests. This is NOT skipped to test a failed test case. See the differences?

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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");
droplet’s picture

Gábor,

You lost and still not jump out of the incorrect patch in that issue. Again, this is 2 isolated issues. (@drpal made a wrong comment)

Drupal works fine with those eslinted files. After patching, we still testing the SAME thing as before patch and more.

Can we update the testing JS in this issue to keep the escaping intact.

This is what I'm doing now on this issue. (move to .fake)

All of your suggestions have already covered in my patch above.

We defined a test and doing string comparsion:
Single Quote \'Escaped\' t

Correct:
JS:
Drupal.t('Single Quote \'Escaped\' t');

PHP matches:
Single Quote \'Escaped\' t

Result: GREEN, this is matched with the test case. Two strings are identical.

Incorrect:
Incorrect patch:
Drupal.t("Single Quote 'Escaped' t");

PHP matches:
Single Quote 'Escaped' t

Result: RED, the string does not match the test case. Now, we found we made an INCORRECT patch.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.28 KB

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).

Status: Needs review » Needs work

The last submitted patch, 26: 2889600-escape.patch, failed testing. View results

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.32 KB

Post phpunit conversion variant so it applies.

droplet’s picture

You made things really wrong. You could call your patch adding a mixed single & double test. But I think it's useless.

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

droplet’s picture

And this is JavaScript syntax, even PHP is same. Only 2 (4) formats:

" ' "
" \" "

' " '
' \' '

You made it mixed:

" ' \" "

is equal to test it separately

" ' "
" \" "

If we intented to add mixed test. It should be below, not modify every lines:

Drupal.t("Mixed Quote ' \" ");
Drupal.t('Mixed Quote \' " ');
Gábor Hojtsy’s picture

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?

droplet’s picture

Gábor Hojtsy

You right! Then I don't understand why you don't get my points... (or I don't get your point)

We should ask nod_ for help. Needs someone to explain it in a new way :)

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
33.31 KB

droplet’s picture

FileSize
60.99 KB

Well. You point is my comment #17. Let's do in a new issue thread. The additional tests can be no escaped quotes. Basically, only whitespace testing. When we chose BabelJS, we trust babeljs itself test engine also. It won't modify the STRING CONTENT.

Besides that, most of time, we able to draw an equals sign

droplet’s picture

We able to enhance the test cases like this:

Drupal . t ( 'wird spacing' ) ;
Drupal  .   t   (   'tab'   )   ;
Drupal
    .
        t
            (
                'tab with newline tabs'
                    )
                        ;
Drupal
    .
        t
             (
                 'mixed tab with spacing with newline tabs'
                      )
                         ;

(imagine large spacing is TAB, I don't know how to add TAB here.)

Abouslotely a new task.

droplet’s picture

Wim Leers’s picture

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.

drpal’s picture

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.

droplet’s picture

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

Hmmm... not really I think... Since we only care if it parses the raw STRINGs (translatable string).
EDITED: BabelJS will not change the String literals, only remove the spacing in this case.

That's why ESLint autofix will fail the bots.
EDITED: ESLint autofix will change the String literals. To remove escape character.

The test data is static data already. We need not validate the test data.

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.

I'm fine with it and it's my preferred way, my first patch of this issue :) and we need not change babel.rc. We will remove ES6 version.

The reason to rename it to .fake is bypass committers githook:
https://www.drupal.org/node/2818825#comment-12092438

Gábor Hojtsy’s picture

Issue tags: -blocker

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 :)

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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

droplet’s picture

Easier life, even neater :P

Please alter my code comments directly if it doesn't explain things well and upload a patch. :P

droplet’s picture

Title: [regression] Restore locale_test.js and keep testing processed JS file » [regression] Restore \LocaleJavascriptTranslationTest test coverage and keep testing processed JS file
Wim Leers’s picture

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.

Wim Leers’s picture

I think we can proceed here?

Wim Leers’s picture

Priority: Normal » Major

Per @xjm, promoting to major.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.54 KB

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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.2 KB
1.16 KB

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.

drpal’s picture

Status: Needs review » Reviewed & tested by the community

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