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

Test failures in latest woofmark update due to ES6 inclusion #607

Open
jywarren opened this issue Sep 1, 2020 · 9 comments · May be fixed by #614
Open

Test failures in latest woofmark update due to ES6 inclusion #607

jywarren opened this issue Sep 1, 2020 · 9 comments · May be fixed by #614
Milestone

Comments

@jywarren
Copy link
Member

jywarren commented Sep 1, 2020

In #597 we started seeing test failures - @keshav234156 opened #602 to revert.

However, the test failures don't trigger a Travis failure, so we weren't seeing them; see this for example:

https://travis-ci.com/github/publiclab/PublicLab.Editor/jobs/379516209

     ReferenceError: Can't find variable: PL in file:///home/travis/build/publiclab/PublicLab.Editor/spec/javascripts/button_spec.js (line 7) (1)
     Expected false to be true. (2)

It looks like potentially a syntax error in our Jasmine tests, under jasmine:publiclabeditor.

The commits were:

Bumps woofmark from 12e2c1d to 39237b7.

  • 39237b7 heading test (#34)
  • 7a47ab7 Fix .gitpod.yml syntax (missing '-' in tasks) (#28)
  • 3a346a6 Bump jest from 26.1.0 to 26.2.2 (#36)
  • a5b9afa Bump browserify from 8.1.0 to 16.5.2 (#38)
  • a35205a Bump jshint from 2.11.1 to 2.12.0 (#37)
  • 8160220 Replacing uglify-js with Terser (#29)
  • e6f56ea fix bold texts loosing formatting after converting to markdown and back to wy...
  • e9e7bb4 add dependencies for tests and modify jest.config (#17)
  • 9bd0750 Install gitpod for auto-built previews on each PR (#16)
  • 4f8d765 Merge pull request #13 from NitinBhasneria/latestChanges
  • Additional commits viewable in compare view

Let's take a close look and see what could have caused this. Notice that Jest tests did not fail.

@jywarren
Copy link
Member Author

jywarren commented Sep 1, 2020

Is this due to Jasmine being unable to read es6, perhaps? @sagarpreet-chadha @Shulammite-Aso @shreyaa-sharmaa @NitinBhasneria @keshav234156 - any thoughts? I notice that we added Terser to woofmark in the meantime?

@keshav234156
Copy link
Member

keshav234156 commented Sep 1, 2020

Actually it's difficult to find as we are able to find only when we integrate it with Editor. It's due to an update that we have made in woofmark and it doesn't get noticed when we test them separatly
@jywarren Do you know any way we can test this?

@jywarren
Copy link
Member Author

jywarren commented Sep 1, 2020

I'm not sure ... the fact that Jasmine is not finding it at all seems to be a big hint. And yet it passes Jest tests.

We haven't really changed the Jasmine setup, i think? https://github.com/publiclab/PublicLab.Editor/tree/main/spec/javascripts (one change 18 days ago)

This to me suggests that it's a difference in the environment somehow. I know Jasmine has trouble with ES6 and we don't have Babel installed to deal with this... ?

Also noting connection to a previous issue with Travis here which ended up being related to Node version: #439 - and there, we asked @HarshKhandeparkar and @VladimirMikulic for help. If we get really stuck let's see if they have suggestions, as they are really good at this stuff?

Thanks!

@sagarpreet-chadha
Copy link
Contributor

I agree with @jywarren , it maybe due to es6 usage in upstream library - woofmark.
I saw that we are now telling jshint that the code is written in es6 here:
https://github.com/jywarren/woofmark/blob/39237b798308e6855acde1a938c18a8e4da62fce/.jshintrc#L2

Maybe this is causing issue?

@sagarpreet-chadha
Copy link
Contributor

I think it is due to use of const keyword which is es6 construct (I only asked asked people to change var to const in some PR's)

@sagarpreet-chadha
Copy link
Contributor

We are using const and even let also here jywarren/woofmark@e6f56ea

@Shulammite-Aso would you like to test if this is the issue?

There are 2 things we can do now in woofmark:

  1. Remove let and const everywhere and then try installing this branch in PL.editor by changing package.json (linking woofmark to your branch) and then running Jasmine tests locally.

  2. Setup a transpiler like babel which will first convert ES6 to lower ECMA version and then later bundle them into /dist folder. (This we should do anyways)

Thanks 😄

@jywarren
Copy link
Member Author

jywarren commented Sep 2, 2020

Awesome. I guess I feel like option 2 makes sense, but it is a bit of work. @sagarpreet-chadha has actually done this (although i didn't review it 😭 ) in this PR so you can see how it's usually configured!

https://github.com/publiclab/leaflet-environmental-layers/pull/240/files

@Shulammite-Aso
Copy link
Collaborator

Hi @sagarpreet-chadha @jywarren is anybody working on this yet?
Things must have gotten over my head. I thought i read you were working on this @sagarpreet-chadha ?
Just reading through again and saw that wasn't what you said.

I should try setting this up if nobody is already on it, and hit you up @sagarpreet-chadha if i have any question.

@Shulammite-Aso Shulammite-Aso linked a pull request Sep 13, 2020 that will close this issue
5 tasks
@jywarren jywarren changed the title Test failures in latest woofmark update Test failures in latest woofmark update due to ES6 inclusion Sep 14, 2020
@jywarren
Copy link
Member Author

Thank you so much! we had gotten a little behind on this but your help is SO APPRECIATED. This is a big blocker! Looking at your solution now. Thanks!! 🎉

@jywarren jywarren added this to the v3.1 milestone Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants