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

Make Ospec Flems-friendly #2034

Merged
merged 9 commits into from Nov 30, 2017

Conversation

Projects
None yet
3 participants
@pygy
Copy link
Member

pygy commented Nov 29, 2017

There are a few annoyances when using Ospec in Flems:

  1. It only works in CommonJS environments

  2. It uses console.error, which in Flems prints out the full stack trace (which can be long in Ospec thanks to nextTickish)

  3. It expects to find its own file name in the path (to clean up stack traces), but Flems only provides a hash, that's more of a Flems issue probably.

This fixes 1 and 2

edit: and now 3, and also #2036

@pygy pygy force-pushed the pygy:ospec-umdish branch from b517b12 to 8eb1245 Nov 29, 2017

@pygy pygy requested a review from tivac as a code owner Nov 29, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 29, 2017

Ok, Gandalf :-)

@pygy pygy referenced this pull request Nov 29, 2017

Merged

Merge v1 1 6 and ospec #2032

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 29, 2017

Please note that the second commit is possibly a breaking change since it causes the report to always run async where it was possibly synchronous in the absence of an async test. Not sure if it matters...

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Nov 29, 2017

Yeah, that's a breaking change. ospec@2 here we come XD

@tivac

tivac approved these changes Nov 29, 2017

Copy link
Member

tivac left a comment

Generally fine w/ this, don't love the breaking change but if it really makes your flems life that much easier I guess it's fine.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 29, 2017

@tivac: contrast this (vanilla ospec) with this (with setTimeout)...

For the third issue, there may be a way on our end to detect the path of the current file but it won't be trivial:

Chrome:

' at new init (blob:https://r.flems.io/fec76e22-06fc-49ab-9da0-463dd47d7a6d:5:14)'

Firefox:

'init@blob:https://r.flems.io/88cd211c-dfe2-1344-8b6a-1c2bd3023f61:5:14'

Safari:

'init@blob:https://r.flems.io/72f1d451-db1d-4535-8923-6c1a70229193:5:23' (like Firefox, but with a different column)

Node:

' at init (/Users/pygy/src/mithril.js/ospec/ospec.js:7:13)' (after the UMD-ish addition, no new init anymore as IIFE)

this was putting the throw() inside the init factory, but when called as o.new, it gives yet another trace entry (Node):

' at Function.init [as new] (/Users/pygy/src/mithril.js/ospec/ospec.js:7:13)'

If IE/edge have similar traces, we may have it as the first line that matches /\/(.*?):\d+:\d+/...

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 29, 2017

It seems to work so far (Node/Firefox/Chrome/Safari).

Could someone try the flems above in IE/Edge and report if the error log points to the right lines?

Edit: made the regexp Windows-friendly as well (backslash).

@pygy pygy force-pushed the pygy:ospec-umdish branch from 776358a to 6df5eba Nov 30, 2017

@@ -183,18 +187,6 @@ o.spec("ospec", function() {
})
})

o.spec("stack trace cleaner", function() {

This comment has been minimized.

@pygy

pygy Nov 30, 2017

Member

The new cleaner expects that the stack comes from within ospec, with at least one framework function on the stack before we reach user code, breaking this test. I've thus moved it to the new reporter test that provides real ospec stack traces.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 30, 2017

Actually, for the stack trace cleaner, starting from the bottom of the stack would be better still but that's for tomorrow.

@pygy pygy force-pushed the pygy:ospec-umdish branch from 6df5eba to 5c25ea1 Nov 30, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 30, 2017

Aargh, that doesn't work either in some scenarios...

@pygy pygy force-pushed the pygy:ospec-umdish branch from 5c25ea1 to b84e093 Nov 30, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 30, 2017

At last, I think this is reliable.

I changed the error field in the report function to point to the error object rather than just the stack trace. the undocumented o.clearStackTrace() now takes an error object rather than a stack trace string. So code that was using errorLocation = o.clearStackTrace(e.error) will still work, but other uses of either clearStackTrace or e.error will break obviously.

Pinging @zyrolasting who introduced that change , which hasn't been released yet on NPM.

@zyrolasting

This comment has been minimized.

Copy link
Contributor

zyrolasting commented Nov 30, 2017

I understand the lack of love for the breaking change only because adding the reporting function meant making all results part of the public API. I do support adding the full error object if only to improve source-map-support integration.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 30, 2017

@zyrolasting changing e.error isn't technically a breaking change because your patch hasn't been released yet on NPM. I gave you the heads up about the update because I supposed that you were already relying on the API introduced there in your own code.

The breaking change is the o.run() change from zalgo to always async.

Before, if none of your tests were async you could do

o.run()
console.log("printed after the report")

Not anymore after this PR.

We may want to provide o.report

to enable

o.run(results => {
  o.report(results)
  console.log("printed after the report")
})
@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 30, 2017

There, now the reporter is exposed as o.report(results) and it returns the number of errors.

I almost moved the process.exit(1) call to the binary, but then I realized that doing so would prevent extra instances created by o.new() from exiting on error by default. Enough breakage already...

@pygy pygy force-pushed the pygy:ospec-umdish branch from 89c64d8 to 3ac17d0 Nov 30, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 30, 2017

I think I'm done :-)

@pygy pygy merged commit 956bdc5 into MithrilJS:next Nov 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

pygy added a commit that referenced this pull request Nov 30, 2017

pygy added a commit to pygy/mithril.js that referenced this pull request Dec 8, 2017

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