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

migrate nodeunit to jasmine #1336

Merged

Conversation

axe312ger
Copy link
Contributor

To sync the test tools we have, this PR migrates from nodeunit to jasmine for the http adapter to have the jasmine framework as only test framework.

Requires #1335 to be merged first, probably also needs upgrading the Karma tests too

Part of #1333

@Khaledgarbaya Khaledgarbaya added this to the v1.0.0 milestone Feb 5, 2018
@Khaledgarbaya Khaledgarbaya self-assigned this Feb 5, 2018
@axe312ger axe312ger force-pushed the test/migrate-nodeunit-to-jasmine branch from 4520ce7 to 46b796b Compare February 6, 2018 09:39
@montogeek
Copy link

What do you think about using Jest?

@zcei
Copy link
Contributor

zcei commented Feb 6, 2018

While I have a general tendency to like Jest, I haven't really worked with it yet - while migrating the karma specs I did a small research and read it would be quite painful to have Jest running in a Browser environment via Karma.
So @axe312ger decided to have the unit tests use the same lib as the specs, so we don't mix & match test suites.

@axe312ger
Copy link
Contributor Author

Jest would have been the way to go IMHO, but since Axios is running most of the tests in several browsers and karma-jest is not a thing yet (see jestjs/jest#848) we have to go for something else.

I would be totally in to migrate to jest as soon they properly support karma/running it in the browser. Would even be kind of a no brainer thanks to wonderful https://github.com/skovhus/jest-codemods

@montogeek
Copy link

What about using Jest + Puppeteer?

@axe312ger
Copy link
Contributor Author

Puppeteer would give us only a chromium backend while the lib still should be checked in IE, Edge, FF and so on.

For local testing puppeteer could be a good option to speed things up.

@Khaledgarbaya
Copy link
Collaborator

Jest also does not support the browser testing

@Khaledgarbaya Khaledgarbaya self-requested a review February 6, 2018 13:16
@Khaledgarbaya Khaledgarbaya mentioned this pull request Feb 6, 2018
48 tasks
@Khaledgarbaya Khaledgarbaya added this to To Do in 1.0.0 via automation Feb 9, 2018
@Khaledgarbaya Khaledgarbaya moved this from To Do to In progress in 1.0.0 Feb 9, 2018
Copy link
Collaborator

@Khaledgarbaya Khaledgarbaya left a comment

Choose a reason for hiding this comment

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

Looks great overall, only one comment


axios.get('http://localhost:4444/', {
try {
await axios.get(testUri, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using async await will require us to use different configs than the library which may introduce some confusion.
It would be nice to use Promises for now. To align the feature use across the library

Copy link
Collaborator

@Khaledgarbaya Khaledgarbaya left a comment

Choose a reason for hiding this comment

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

Great job

@Khaledgarbaya Khaledgarbaya merged commit d1101f4 into axios:release/1.0.0 Feb 9, 2018
1.0.0 automation moved this from In progress to Done Feb 9, 2018
@Khaledgarbaya Khaledgarbaya deleted the test/migrate-nodeunit-to-jasmine branch February 9, 2018 10:56
zcei pushed a commit to contentful/axios that referenced this pull request Apr 26, 2018
@axios axios locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
1.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants