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.

Sign up for GitHub

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

Jump to bottom

Add ability to compile tabtest in the cloud and run it locally #1602

Merged
merged 5 commits into from
Dec 26, 2018
Merged

Add ability to compile tabtest in the cloud and run it locally #1602

merged 5 commits into from
Dec 26, 2018

Conversation

evgenykochetkov
Copy link
Contributor

Closes #1543

@evgenykochetkov evgenykochetkov self-assigned this Dec 25, 2018
knopki
knopki requested changes Dec 25, 2018
View reviewed changes
Copy link
Member

@knopki knopki left a comment

Choose a reason for hiding this comment

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

Can you pull runtime.js from service again? I have a suspicion that we both made edits to it in parallel. Just to be sure.

packages/xod-cloud-tabtest/test/endToEnd.spec.js Outdated Show resolved Hide resolved
@knopki
Copy link
Member

knopki commented Dec 25, 2018

Not sure, do you send main.cpp and catch.hpp in payload? Then can be dropped (for speed up) because backend service use own bundled versions.

nkrkv
nkrkv requested changes Dec 26, 2018
View reviewed changes
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

packages/xod-cloud-tabtest/README.md is missing

nkrkv
nkrkv reviewed Dec 26, 2018
View reviewed changes
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

I’d put the runtime.js file to some other location rather than ./src/ because it is not a source in the usual sense. Maybe ./vendor/ or something.

nkrkv
nkrkv requested changes Dec 26, 2018
View reviewed changes
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

Can we have the green progress bar on the pane displayed?

  • 25% - Generating C++ code...
  • 50% Building...
  • 75% Running tests...

nkrkv
nkrkv reviewed Dec 26, 2018
View reviewed changes
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

Electron, Linux, I’m clicking Run:

Generating C++ code...
Building...
Running tests...
No WebAssembly support found. Build with -s WASM=0 to target JavaScript instead.

🤣

Not so funny, actually. Here’s what I found https://kripken.github.io/emscripten-site/docs/getting_started/FAQ.html#what-is-no-webassembly-support-found-build-with-s-wasm-0-to-target-javascript-instead-or-no-native-wasm-support-detected

Am I have “wrong” electron or NodeJS? @knopki can you run the tests just fine on your Linux?

knopki
knopki reviewed Dec 26, 2018
View reviewed changes
packages/xod-cloud-tabtest/src/compile.js
JSON.stringify.bind(JSON),
filesMap => ({
fqbn: 'wasm:tabtest',
options: {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe put this to vendor dir with runtime.js? Because this is very static options.

@knopki
Copy link
Member

knopki commented Dec 26, 2018

@nkrkv Same error. I think we need electron >= 1.7.0 or some magic startup argument (as old nodejs and chromium had).

@nkrkv
Copy link
Member

nkrkv commented Dec 26, 2018

@knopki would you help @evgenykochetkov with the research?

@knopki
Copy link
Member

knopki commented Dec 26, 2018

@evgenykochetkov @nkrkv --expose-wasm (from nodejs) and --enable-webassembly (from chromium) and --js-flags="--expose-wasm" doesn't help. Unfortunately, electron update is must. WebAssembly support added and enabled by default in 1.7.0, but we are on 1.6 branch.

brusherru
brusherru approved these changes Dec 26, 2018
View reviewed changes
Copy link
Contributor

@brusherru brusherru left a comment

Choose a reason for hiding this comment

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

It works fine on MacOS! 👍

knopki
knopki approved these changes Dec 26, 2018
View reviewed changes
Copy link
Member

@knopki knopki left a comment

Choose a reason for hiding this comment

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

Works fine on leenooks

nkrkv
nkrkv reviewed Dec 26, 2018
View reviewed changes
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

Works fine for me too now. And I can bet the overall UI performance is snappier!

But may we have the green progress bar as well, please?

@evgenykochetkov
Copy link
Contributor Author

Just added the progress bar.

nkrkv
nkrkv approved these changes Dec 26, 2018
View reviewed changes
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

Very cool! 👍

@evgenykochetkov evgenykochetkov merged commit 5d09a5f into master Dec 26, 2018
@evgenykochetkov evgenykochetkov deleted the feat-1543-running-tabtests branch December 26, 2018 13:45
@evgenykochetkov evgenykochetkov mentioned this pull request Dec 26, 2018
Merged
This was referenced Aug 1, 2022
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brusherru brusherru brusherru approved these changes

@knopki knopki knopki approved these changes

@nkrkv nkrkv nkrkv approved these changes

Assignees

@evgenykochetkov evgenykochetkov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@evgenykochetkov @knopki @nkrkv @brusherru