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

Update to karma-chrome-launcher, removal of GitPod, refactor of export example to remove jQuery #716

Merged
merged 42 commits into from
Sep 1, 2020

Conversation

sashadev-sky
Copy link
Member

@sashadev-sky sashadev-sky commented Aug 18, 2020

Fixes #685 (<=== Add issue number here)
Fixes #713
Fixes #710
Fixes #642
References #681

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updates
  • @mention the original creator of the issue in a comment below for help or for a review

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

===========================

  • Remove phantomjs dependencies
  • Bump karma, mocha, karma-mocha, sinon and karma-coverage
  • Run chrome launcher locally and in travis.
  • Remove support for Gitpod - it cannot run chrome launcher locally without the --no-sandbox option, so we are better off just removing support for it until this issue is resolved or we change our testing framework to one that doesn't require a real browser env.
  • Update webpack.config.js and WEBPACK.md to remove jquery and promise-polyfill from vendor.js (no longer needed), leaflet.css from vendor.css (we don't include leaflet.js for vendor.js to allow any leaflet version so should do the same with css), and update port 8080 -> 8081 (8080 is typically in use in my dev env. so inconvenient)
    • Update example html files accordingly
    • Update src/edit/DistortableCollection.Edit.js to use ES6 syntax ... trying to make export.html file work correctly but stuck on this
  • Update TESTING.MD
  • add leaflet as devDependency for running tests
  • bump version 0.20.7 -> 0.21.7
    ===========================

@sashadev-sky
Copy link
Member Author

sashadev-sky commented Aug 25, 2020

@jywarren updated export.html but can't make it work.. also seeing it doesn't work in our gh-pages either so perhaps it never worked? The POST req to //34.74.118.242/api/v2/export/ times out in both scenarios. Going to say fixing that part is out of scope for this PR because i have no idea what is wrong! Just made sure here that we aren't using ajax anymore, etc.

Note I tested the startExport function updates on my own gh-pages page https://sasha.mapknitter.org/examples/select.html so I know its not any src code changes.

Where did this url come from ? //34.74.118.242/api/v2/export/

@sashadev-sky
Copy link
Member Author

sashadev-sky commented Aug 28, 2020

@jywarren @VladimirMikulic @rexagod ok going to hold on this for now. I got headless chrome working in travis and locally, but unfortunately the only way to make it work in Gitpod is by adding the --no-sandbox option, which is a security risk we prob shouldn't take?

We could run PhantomJS in gitpod, but it's not ideal because its a) deprecated and b) not compatible if we bump other packages like sinon which could really use the update.

Soo options are:

  1. drop gitpod support for the meantime
  2. keep gitpod support just don't support run the tests in there (travis does that anyway already)
  3. change our entire testing framework to something like the image sequencer setup, so we don't need a real browser environment.

What are your guys thoughts? I sorta just lean towards 1, i figure theyll work out this sandbox stuff eventually and we can readd then. 3 would be awesome but time consuming!

@MLefebvreICO
Copy link

How come Chromium with puppeteer would resolve the security no sandbox issue?

I don't know if it will resolve that, but it will allow you to replace PhantomJS easily without having to configure a test image with a Chromium that works properly in such an environment.

We were using a docker image on our end with a Chromium installation, but after a Chromium recent release the tests were not executing at all, because of an issue with headless environment. I don't recall what was the issue, but using the Chromium binary from puppeteer solved the issue for us.

@sashadev-sky
Copy link
Member Author

@MLefebvreICO puppeteer still presents the same issue with the sandbox :( so what do you think would be the most move from the other options i mentioned before?

@publiclab publiclab deleted a comment from gitpod-io bot Sep 1, 2020
@sashadev-sky
Copy link
Member Author

@jywarren ok I decided to drop Gitpod support in the mean time. I'll open an issue regarding updating our testing framework! I'm going to merge this to fix some of our CI issues

@sashadev-sky sashadev-sky merged commit eec2348 into publiclab:main Sep 1, 2020
@sashadev-sky sashadev-sky mentioned this pull request Sep 1, 2020
@jywarren
Copy link
Member

jywarren commented Sep 15, 2020

Hi! Sorry, just catching up here. I believe --no-sandbox is standard usage for headless chrome, so I'd appreciate adding that back in! The Gitpod integration in #680 is really really useful and i think critical for both broadening our contributor base and also for manually testing out PRs.

You can read about --no-sandbox here and it also offers a workaround -- https://developers.google.com/web/updates/2017/04/headless-chrome#faq and theintern/intern#878 (comment)

@sashadev-sky can you add it back in, pls? Thanks!!! (resolved in #750, follow-up in #735)

success: function(data) { opts.handleStatusRes(data, opts) }, // this handles the initial response
});
}
const fetchStatusUrl = (mergedOpts) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look good, but without the upstream server working, I guess I would not have merged these changes without testing it against the cloud export server!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate please? I have some catching up to do! Is there anything that i should focus on immediately?

@jywarren jywarren changed the title Update to karma-chrome-launcher Update to karma-chrome-launcher, removal of GitPod, refactor of export example to remove jQuery Sep 15, 2020
@jywarren
Copy link
Member

@jywarren updated export.html but can't make it work.. also seeing it doesn't work in our gh-pages either so perhaps it never worked? The POST req to //34.74.118.242/api/v2/export/ times out in both scenarios. Going to say fixing that part is out of scope for this PR because i have no idea what is wrong! Just made sure here that we aren't using ajax anymore, etc.

Note I tested the startExport function updates on my own gh-pages page https://sasha.mapknitter.org/examples/select.html so I know its not any src code changes.

Where did this url come from ? //34.74.118.242/api/v2/export/

Hi Sasha - just replying on this too -- so, i think it's really important to ensure we don't break export functionality -- i believe the upstream cloud cluster is just offline right now, so I'm going to open an issue to ensure we test this new code against a working cloud export service.

Actually, can we aim to do these kinds of changes in individual feature-specific PRs in the future? That can help us solve things separately without having to merge things we aren't yet sure of like the exporter, and not have them hold up the overall release. Thank you so much!

This is a really amazing PR! Thanks for your fantastic work on it!

@sashadev-sky
Copy link
Member Author

@jywarren yes definitely on board with that, will check out #751 now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants