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

Fix test cases of //website/app_test.js #889

Merged
merged 2 commits into from Oct 4, 2018

Conversation

3 participants
@kt3k
Copy link
Contributor

kt3k commented Oct 3, 2018

This PR tries to fix failed test cases in //website/app_test.js.

(CI seems skipping these tests. See #888 for details)

@kevinkassimo

This comment has been minimized.

Copy link
Contributor

kevinkassimo commented Oct 3, 2018

Was discussing about this in #885. Like #882 failed to update tests

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Oct 3, 2018

@kevinkassimo Thanks for the reference.
I think this PR fixes the test cases excluded in #885 (maybe introduced in #882)

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Oct 3, 2018

Thanks @kt3k looks good. Two comments:

  • I want to land the timers patch (#885) first.
  • The tests for benchmarks could be improved further still. Adding the fetch_deps syscall count has now required three changes: b7fd6e9, 545109c, and this patch. Ideally if I made only the first change to tools/benchmark.py and ran the tests, I would get a failure indicating that I needed to update website/app.js as well. (Or better yet, website/app.js automatically detected the columns for each benchmark.) This is beyond the scope of what you're doing here - but I'm mentioning it in case you or @kevinkassimo would like do a refactor.
@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Oct 3, 2018

@ry OK. No problem. I'll update this PR after #885 landed 👍 And going to consider the latter part as well.

@ry ry referenced this pull request Oct 3, 2018

Merged

Upgrade JS deps #891

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Oct 3, 2018

#885 has landed. Please rebase.

@kt3k kt3k force-pushed the kt3k:fix_website_app_test branch from 5b47b69 to 1d2cddd Oct 4, 2018

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Oct 4, 2018

Rebased and restored fixed test cases.

@ry

ry approved these changes Oct 4, 2018

Copy link
Collaborator

ry left a comment

LGTM - thanks!

@ry ry merged commit e5e7f0f into denoland:master Oct 4, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@kt3k kt3k deleted the kt3k:fix_website_app_test branch Oct 4, 2018

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