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

Jasmine 3 runner #57

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Jasmine 3 runner #57

wants to merge 3 commits into from

Conversation

aminya
Copy link
Member

@aminya aminya commented Jul 19, 2020

No description provided.

@aminya
Copy link
Member Author

aminya commented Jul 20, 2020

@DeeDeeG In the renderer tests (tests affected by this pull request), apm is not detected for some reason. Should we put it on the path? This is fixed in Jasmine3 runner 5.0.0

@UziTech Any thoughts about this? If I recall correctly, Jasmine 3 is faster in running the tests, and it has the capability to run the tests in parallel. If we can convert some of the expensive tests (e.g. pane-spec.js) to Jasmine 3, we can solve the timeout issues

vendor/jasmine3-runner.js Outdated Show resolved Hide resolved
@UziTech
Copy link
Member

UziTech commented Jul 20, 2020

I don't believe Jasmine v3 runs tests in parallel although that may be something that is added in the next release (jasmine/jasmine-npm#153)

The biggest benefit I see in using Jasmine v3 over v1.3 is being able to use newer async features and matchers.

@aminya
Copy link
Member Author

aminya commented Jul 20, 2020

Something strange is happening here: https://dev.azure.com/atomcommunity/atomcommunity/_build/results?buildId=340&view=logs&j=0c24d9d5-0e27-5404-37e0-91cb4d692d0c&t=3786b6db-cb02-5fcf-294f-324dd576bffa

Although all the tests have passed, the code has exited with 0 number.

@UziTech
Copy link
Member

UziTech commented Jul 20, 2020

hmm 🤔 the only thing I can think is if there is an async call that isn't awaited and fails. so the test finishes with async functions still running that fails an expect.

@aminya
Copy link
Member Author

aminya commented Jul 21, 2020

hmm 🤔 the only thing I can think is if there is an async call that isn't awaited and fails. so the test finishes with async functions still running that fails an expect.

Which async call? Here all the tests failed. async.parallel just runs child processes independently and asynchronously, and in the end reports the errors. It is done by async lib. We can switch to async.series for checking

@UziTech
Copy link
Member

UziTech commented Jul 21, 2020

Which async call?

I haven't looked through the tests, it was just an idea of something that could make it show 0 failures and still fail.

e.g.

// this test will not be marked a failure but still fail
it("test", () => {
  setTimeout(() => {
    expect(1).toBe(2);
  }, 1000);
});

@aminya
Copy link
Member Author

aminya commented Jul 26, 2020

There should be no difference here. I am just using the same Jasmine 1 using the new Jasmine 3 runner. 🤔 There is no async call in the tests themselves or in the test script. They are child processes that call the test like atom --test spec/something.js.

@DeeDeeG

This comment has been minimized.

@aminya
Copy link
Member Author

aminya commented Oct 16, 2020

I found why the tests fail here.

UziTech/atom-jasmine3-test-runner#166 (fixed) and UziTech/atom-jasmine3-test-runner#167 (hopefully will be fixed) are the two of the reasons.

@UziTech
Copy link
Member

UziTech commented Oct 16, 2020

UziTech/atom-jasmine3-test-runner#167 only deals with the testPaths option. since atom doesn't use the testPaths option UziTech/atom-jasmine3-test-runner#167 won't do anything to help make the tests pass.

If the tests are failing because there are no tests in some packages then we should make tests for those packages.

@aminya aminya closed this Jul 22, 2021
@aminya aminya closed this Jul 22, 2021
@aminya aminya reopened this Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants