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

Switch to tape or mocha/karma? #115

Closed
mindplay-dk opened this issue Dec 11, 2019 · 30 comments
Closed

Switch to tape or mocha/karma? #115

mindplay-dk opened this issue Dec 11, 2019 · 30 comments

Comments

@mindplay-dk
Copy link
Contributor

What do you think about switching from jest to tape?

I've got a preliminary setup here:

master...mindplay-dk:use-tape

This is unfinished - tape is much simpler than jest, but it doesn't come with "batteries included", so it takes more work to set this up.

For now, I have Babel with JSX and async support working - this seems already much safer, because I can clean up JS-DOM after each test, which means that timeouts get flushed, which means tape can catch assertions that run after the promise resolves. (the commented-out lines demonstrate this.)

I still need to set up code coverage and XML reporting for CI integration - before I can port the actual tests, so a lot of work left.

Do you think I should continue with this?

@yisar
Copy link
Collaborator

yisar commented Dec 11, 2019

I've never used test framework other than jest. I think our tests are running well now, and it may not be better to change the framework, so I think this may be able to pause?

I had a hard time understanding our test code, and I think most of our problems have been solved.
We can complete the rest of the test cases.

Is tape more popular than jest?

If so, is it possible for us to use it in fre2?

@mindplay-dk
Copy link
Contributor Author

Well, Jest is by far the most popular of those two - but I've seen many people asking/commenting on the same issues we've run into, and the situation doesn't seem to be improving.

I suspect that Jest is mostly popular because it's the de-facto standard test-framework for anyone who uses React - they have facilities specifically designed for that, so... many React users -> many Jest users.

My main concern about Jest is, I don't feel like I can trust it - I don't even know if it's running all the tests or hitting all the assertions... things often look fine, and then I discover down the line that I forgot an await statement in a test or something and half the test wasn't running at all.

I don't know if all our tests are actually passing for the right reasons - as you saw, it's completely possible to write a test with a loop full of assertions that Jest just ignores because you called done.

From what I can tell, tape is much more reliable in that way at least.

Our test code wouldn't really change much at all - tests are still just functions being passed to a test function, assertions may have slightly different names but basically work the same, and we're only using one or two of them.

Anyhow, let's leave this here for now and think about it...

@yisar
Copy link
Collaborator

yisar commented Dec 11, 2019

I'm sorry for using done() by mistake. Yes, as you said, sometimes I don't know why the test passed. I think we need a proper time to change the test framework.

In fact, I have little experience in testing. Thank you very much for your guidance and help.

I think we can open this issue and come back after settling for a while……

@yisar
Copy link
Collaborator

yisar commented Dec 12, 2019

hey, I think we can continue this issue now, thank you for your proposal.
tape is simple and cute, if the work is too heavy, we can migrate in stages.

@mindplay-dk
Copy link
Contributor Author

mindplay-dk commented Dec 12, 2019

Okay, so I think the first step is just to get the change from #117 ported over to tape and see if that changes anything.

If that doesn't help, we'll know the problem isn't Jest (which could mean it's JS-DOM or something else we haven't thought of) and that'll help us decide whether to proceed with tape or what to do next.

@yisar
Copy link
Collaborator

yisar commented Dec 12, 2019

Ok 😉

@mindplay-dk
Copy link
Contributor Author

Man, it's worse than I thought...

I was using jsdom, which is what jest uses, but I was using another package jsdom-global to expose (and clean up) all the DOM browser globals - and it turns out performance, which we need for scheduling, isn't supported, and jsdom-global is most likely a dead project anyhow, so...

I dug into jest, which has it's own implementation of a browser environment based on jsdom, but this was purpose-built for jest and isn't really portable.

I also poked around in Jest's fake implementation of timers to answer another question: what happens to timers that run too late? It turns out, these get destroyed and won't finish executing if the JS-DOM instance is cleaned up.

This explains why a line like this one actually executes, even though it's being run after the JS-DOM instances has been cleaned up: the fake timers (if they're even fake?) are an entirely different implementation than the one in Jest.

These differences could give us even more problems rather than solving them.

At this point, I'm sort of fed up with jsdom and I don't believe it's worth the effort - I think we have to figure out how to get a proper headless browser environment running for the tests... jsdom and jest are likely too unrealistic on some of the important points around scheduling - and even if we could get this to work, as mentioned, it doesn't prove that this works in a real browser.

We could still use tape, but maybe at this point it's better to look at something more mainstream like mocha and karma, which is what jest is based on anyhow - so our tests wouldn't need to change much, and we can probably reference an existing project for many of the solutions, for example the package.json of preact, as they're already solving all of the problems we need to solve. (JSX, ES6, headless browser, etc.)

I think maybe jest is a good choice if you're building with a standard frameworks (React, Angular, VueJS) but at this point I'm not convinced it's a good choice for testing a UI framework - I think mocha itself is probably a better choice for something more low-level, and at least Angular, Vue and Preact all use karma for the browser stuff.

This is turning out to be quite the adventure, ha ha. 😂

@mindplay-dk mindplay-dk changed the title Switch to tape? Switch to tape or mocha/karma? Dec 12, 2019
@yisar
Copy link
Collaborator

yisar commented Dec 12, 2019

If only performance is not supported, then polyfills can be found, and I don't think js-dom is a good choice.
We can refer to the practice of preact, multiple libraries are used at the same time.
Anyway, I don't think jest is a good choice for testing js framework now.

@mindplay-dk
Copy link
Contributor Author

I've gotten one test ported to mocha, and it passes - changes from jest test-format are pretty minor, but the built-in assertions provided by node were lacking, so I also added chai, which provides nice assertions. I could easily port all the tests, but this first test doesn't have any browser-dependency - there's no point in porting everything until we've got a working browser environment integration.

So I've attempted to create a repeatable headless Chromium setup with mocha. I have the integration working - it's attempting to launch Chromium, but currently fails because of a missing dependency.

I've also tried with the puppeteer package, and it fails just the same - and people seem to accept the solution that all dependencies have to be manually installed, so I don't know... maybe there's no such thing as an npm installable Chrome environment?

I don't think preact have that either, so maybe that's just the sort of pain that the whole JS community puts up with and solves on every individual system? 🤷‍♂️

@yisar
Copy link
Collaborator

yisar commented Dec 14, 2019

I don't think our test needs to be so troublesome. It doesn't need headless browser(such as puppeteer), but only Dom and BOM.
May be like this:

import undom from 'undom'
import performance from 'performance-polyfill'

We only need a small Dom and BOM to simulate the variables used by fre, such as documentrequestAnimationFrameperformance.now()

The difference between now and js-dom is that we can use a third-party library or our own code to implement these APIs.

@mindplay-dk
Copy link
Contributor Author

I wouldn't trust undom or polyfills anymore than I would trust jsdom - the net result of putting together a mock DOM and some polyfills is going to be much the same as jdsom, and it's never going to behave exactly like a real browser in the ways that matter, not for reliable testing of scheduling etc.

It's the same problem as with jsdom - even if you can get it working and get the tests passing, what does that prove? The polyfill for MessageChannel for example can't be implemented in a way where it executes like it does in a real browser.

Frameworks really need to be tested against a real browser.

At the very least, I think we need an option to run the tests on a local browser during development - even if we have to run them with polyfills on the CI server, at least someone can do a real browser-based test on their local system before pushing a change.

@yisar
Copy link
Collaborator

yisar commented Dec 15, 2019

It is hard to test the scheduler, not only fre, but also react, But I think this is not about headless browser. we just have not found a way to test it in node.

@yisar
Copy link
Collaborator

yisar commented Dec 15, 2019

At the very least, I think we need an option to run the tests on a local browser during development

Maybe devtools or this

mindplay-dk added a commit to mindplay-dk/fre that referenced this issue Dec 15, 2019
@mindplay-dk
Copy link
Contributor Author

Well, finally some good news!

I scrapped jsdom-global and manually set up jsdom instead - and got the first test to pass! 🥳

mindplay-dk@ac7e5d9

More good news: this is the reconciliation test that we couldn't get to work with jest, and it's passing with tape now, with stateNumber >= 1 as it was meant to be! I even shaved a few bytes by removing your stand-in for requestAnimationFrame and adding a trivial mock to the test-setup instead.

So it looks like there's a way forward with tape after all.

I'll try to port the rest of the test-suite one of the coming days. 🙂

I will say again though: a passing test with mocks for requestAnimationFrame and others does not guarantee the same test would pass in a browser - mocks and stubs under node can never behave exactly like a real browser, so I still think we should add an option to run the test in a local browser at some point.

For now, I'm happy get rid of jest and get a faster, working test-suite though! 👍

@yisar
Copy link
Collaborator

yisar commented Dec 15, 2019

@mindplay-dk Wow great, look forward to it!

I will say again though: a passing test with mocks for requestAnimationFrame and others does not guarantee the same test would pass in a browser.

I see, so we can only test DOM in node, and then test scheduler through chrome panel in the future.

@mindplay-dk
Copy link
Contributor Author

I see, so we can only test DOM in node, and then test scheduler through chrome panel in the future.

Well, it'll run the full suite of tests - just that those tests will be effectively testing the integration with a bunch of mocks and stubs, whereas what we're really interested in is integration with a browser.

But we'll figure out later how to run the tests without the mocks and stubs. 🙂

@mindplay-dk
Copy link
Contributor Author

Well, I'm stuck again. 😣

The problem is once again testUpdates and the timing issues I've mentioned many times now - there just isn't any reliable way to know when the scheduler is finished, so I'm stuck with update.test.jsx again, and I really don't think there's any way to make this properly testable right now.

To explain the problem, I've isolated a very simple test on a branch.

I've put a bunch of log statements in testUpdates, so you can see the order in which things execute.

    AWAIT  0
    SET CONTENT 0 { type: [Function: Component], props: {}, key: null, ref: null } [ Text { last: null } ]
    NOOP EFFECT
    RUN TEST  0
    ✔ should be equal
    ✔ should be equal
    RESOLVE  0
    RESOLVED  0
    AWAIT  1
    SET CONTENT 1 { type: [Function: Component], props: {}, key: null, ref: null } [ HTMLDivElement {
        last:
         { type: 'text',
           props: [Object],
           op: 3,
           tag: 0,
           parent: [Object],
           sibling: null,
           parentNode: [Circular],
           node: [Text],
           insertPoint: null,
           pendingProps: [Object] } } ]

    ✖ test exited without ending
    -----------------------------

As you can see, the test somehow exits before effect is dispatched - we see SET CONTENT 1, but never RESOLVED 1, and we never see RETURN, so we never reach the end of testUpdates at all.

The update-test itself says await testUpdates(...), so the test shouldn't end until that promise resolves... but we never even reach the TEST ENDS log statement before the script somehow exits.

Maybe this tape-promise package doesn't really work? It seems to work in other cases though - the reconciliation test also does a bunch of updates, and it works fine.

I've spent more than half a day trying to solve it - I have no ideas. 😐

@yisar
Copy link
Collaborator

yisar commented Dec 16, 2019

How should I debug your code? Without debugging, it seems that it is because of content
It shoud like this:

await testUpdates([
    {
      <Compoent value={[]}/>, //give it a props to rerender
      test: ([button]) => {
        is.equal(+button.textContent, 0)
        is.equal(updates, 1)

        // trigger several functional state updates:
        // button.click()
        // button.click()
        // button.click()
      }
    }

I want to debug the code. Is there any way to pull your branche?

@mindplay-dk
Copy link
Contributor Author

it seems that it is because of content

In this particular case, the component doesn't need to be updated by testUpdates - the setState calls inside the onClick handler (commented out for now) will trigger the updates.

But first problem is to get the test to actually run at all - before the script exits.

Is there any way to pull your branche?

You want to add a remote, then check out (not pull) my branch.

@mindplay-dk
Copy link
Contributor Author

You were right - I finally got that test to pass, I just had to use the <Component value={[]}/> hack to bypass the optimization, as you suggested.

Will post an update soon.

@yisar
Copy link
Collaborator

yisar commented Dec 17, 2019

@mindplay-dk In fact, another optimization is hit here. When the state does not change, the component will not be updated.

const content = <Compoent />

setContent(content)
setContent(content) // do not rerender

It should be like this:

setContent(<Component />)
setContent(<Component />)

@mindplay-dk
Copy link
Contributor Author

For the record, a lot of the test issues you've been closing, those tests don't actually work - many of the assertions aren't being run at all... tape doesn't report any errors if you make assertions after the test has ended - those assertions are just ignored.

I had started porting the tests to another framework called zora, after discovering how poorly this works in both jest and tape, but my branch is now 40+ commits behind and probably needs loads of manual merging to get it up to date.

And there are still problems with that branch - for one, it suffers from the same problem as current tests: some tests will pass when run individually, but they will fail when you run them together, due to race conditions. zora provides a solution for this, but it's more work.

Sorry, but I can't keep up - too many things are changing daily, and I can't port and fix the entire test-suite fast enough to finish before my work is already outdated and can't be merged.

I'm going to take a break as I'm wasting too much time on work I can't finish...

@yisar
Copy link
Collaborator

yisar commented Dec 28, 2019

@mindplay-dk Thank you very much, don't rush, take your time. I recently checked that ,I'm sick and I need to be hospitalized. treatment is needed and I need to paused few months. Let's wait together.

@mindplay-dk
Copy link
Contributor Author

Sorry to hear that! I hope you get well soon. 🌻

@wcastand
Copy link

wcastand commented Jan 5, 2021

Just to mention that this is written in Typescript and tape doesn't like Typescript at all. The devs don't want to support it or have anything to do with it from what i've seen in the issues. That might be an issue at some point too.

Jest is bigger but well adapted for UI library like React, Fre, etc
Also for adoption, makes sense to use the same tool as most of the UI library similar to Fre

Just giving my 2 cents here :)
I love tape and use it as much as i can but it definitely has issues when it comes to Typescript and UI library(more setup to make like @mindplay-dk said).

@yisar
Copy link
Collaborator

yisar commented Jan 5, 2021

@wcastand

https://www.npmjs.com/package/@web/test-runner

Do you know test-runner? This is a popular testing tool recently, it supports headless browsers.

The reason fre doesn't want to use jest is because jest cannot accurately simulate browser behavior, which makes our testing more difficult, especially time slicing and tearing.

We need to use a headless browser to ensure accurate behavior. I hope I can switch to test-runner and try it together?

@wcastand
Copy link

wcastand commented Jan 5, 2021

A lot of people use Cypress to test browser behavior, is it what you look for?

Took a quick look at test-runner, seems different than Cypress. Also look pretty nice, using esbuild or buildless setup with ESM which is cool. Worth looking into it for sure :)

I don't know test-runner, i'm no expert in testing framework, especially browser ones like cypress or test-runner. I alway find them super heavy and too time consuming for not much return (talking from a agency/product company point of view, for library like fre it's probably different).

@yisar
Copy link
Collaborator

yisar commented Jan 5, 2021

I alway find them super heavy and too time consuming for not much return.

Yes they are very heavy. The test task is given to CI, and the best result is that developers need not to install huge dependencies.

In order to test the features of concurrent mode, we have to do this, because fre rendering is asynchronous, it needs more test case accuracy than synchronous.

Let me try it next, I hope such a heavy thing can bring benefits.

@wcastand
Copy link

wcastand commented Jan 5, 2021

When i talk about heavy, i'm talking about the time it takes to get them to work and maintained for the dev. Size is not really an issue when it comes to dev tools since it's mostly run on CI like you said.

But in my previous company, they had cypress and maintaining that took more time than making features lol (not even talking about the time it took to run the tests on CI, half the time was cypress tests even in async)

But for fre it's necessary i'd say and will probably be easier to maintain since the API is smaller.

@yisar
Copy link
Collaborator

yisar commented Jun 6, 2021

微信图片_20210606191240

I refactor the algorithm again, and then used a new technique called centralized paints

This is a technology similar to DocumentFragment. It operates DOM in memory and finally paints to the screen at final time.

Now fre is super fast, even faster than vanilla js.

@yisar yisar closed this as completed Jun 6, 2021
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

No branches or pull requests

3 participants