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

Enable almost all tc39 tests #1816

Merged
merged 5 commits into from
Feb 2, 2021
Merged

Enable almost all tc39 tests #1816

merged 5 commits into from
Feb 2, 2021

Conversation

mstoykov
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Jan 25, 2021

Codecov Report

Merging #1816 (97689ae) into master (45fff81) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1816   +/-   ##
=======================================
  Coverage   71.58%   71.58%           
=======================================
  Files         181      181           
  Lines       13939    13939           
=======================================
  Hits         9978     9978           
  Misses       3328     3328           
  Partials      633      633           
Flag Coverage Δ
ubuntu 71.54% <ø> (-0.02%) ⬇️
windows 70.16% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/testutils/minirunner/minirunner.go 76.08% <0.00%> (-4.35%) ⬇️
lib/executor/vu_handle.go 95.23% <0.00%> (+1.90%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45fff81...97689ae. Read the comment docs.

@mstoykov mstoykov force-pushed the enableAlmostAllTc39Tests branch from 9898351 to eaea04d Compare January 25, 2021 15:45
@mstoykov mstoykov marked this pull request as ready for review January 26, 2021 14:16
@mstoykov mstoykov requested review from imiric and na-- January 26, 2021 14:16
@mstoykov
Copy link
Contributor Author

Please go commit by commit ;)

@mstoykov mstoykov force-pushed the enableAlmostAllTc39Tests branch from 351500b to 928e730 Compare January 26, 2021 18:33
imiric
imiric previously approved these changes Jan 27, 2021
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

breaking_test_errors.json is impossible to review 😅, but LGTM otherwise.

Running all of it in ~15m doesn't sound bad considering the scope. Nice work! 👏

for _, path := range pathBasedBlock { // TODO: use trie / binary search?
if strings.HasPrefix(newName, path) {
ctx.t.Run(newName, func(t *testing.T) {
t.Skipf("Skip %s beause of path based block", newName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Skipf("Skip %s beause of path based block", newName)
t.Skipf("Skipped %s because of path based block", newName)

@na-- na-- added this to the v0.31.0 milestone Jan 29, 2021
na--
na-- previously approved these changes Feb 1, 2021
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

LGTM, though I haven't reviewed all of the new errors in any detail... Hopefully we bring them down with further goja updates 🤞

@mstoykov mstoykov dismissed stale reviews from na-- and imiric via b71ee6d February 1, 2021 15:55
@mstoykov mstoykov force-pushed the enableAlmostAllTc39Tests branch from 5474050 to b71ee6d Compare February 1, 2021 15:55
@mstoykov mstoykov requested review from imiric and na-- February 1, 2021 15:56
na--
na-- previously approved these changes Feb 1, 2021
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

LGTM, though please squash when merging - the fewer copies of the huge JSON file we have in the git history, the better...

@mstoykov
Copy link
Contributor Author

mstoykov commented Feb 1, 2021

the fewer copies of the huge JSON file we have in the git history,

git only tracks diffs ... so this is barely a problem, especially given the size of the vendor directory

@mstoykov mstoykov merged commit 080a1ed into master Feb 2, 2021
@mstoykov mstoykov deleted the enableAlmostAllTc39Tests branch February 2, 2021 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants