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

Fixing http tests to avoid hanging when assertions fail #4435

Merged

Conversation

mbargiel
Copy link
Contributor

@mbargiel mbargiel commented Jan 31, 2022

The Mocha tests for the http adapter are asynchronous and rely on Mocha's done callback to be invoked on success - or failure. When it's not called, the tests hang. I identified various places where done was not being called, or was called incorrectly (ie never actually being reachable).

Here are two examples of handling that wasn't correct:

      axios.get('http://localhost:4444/').then(function (res) {
        assert.deepEqual(res.data, data);
        done(); // <-- this is never called if the `axios.get` promise is rejected. 
                // It's also not called if `assert.deepEqual` fails, because it 
                // throws. When `done` is used, Mocha treats the test function 
                // as asynchronous and does not handle exceptions
                // as the end of the test.
      })
        .catch(function (error) { 
          assert.ifError(error); // <-- !? rejected promises should have something passed to them, normally
          done(); // <-- this is never called, unless the promise was rejected with `undefined` or `null`
        });

This PR does not fix all occurrences, but all those from cases similar to the above. A future pass should be done on tests that are event-based to make sure done(error) is called in the proper places.

Ref: mentioned in issue #4318.

Copy link
Contributor Author

@mbargiel mbargiel left a comment

Review pointers

test/unit/adapters/http.js Show resolved Hide resolved
test/unit/adapters/http.js Show resolved Hide resolved
test/unit/adapters/http.js Outdated Show resolved Hide resolved
test/unit/adapters/http.js Show resolved Hide resolved
test/unit/adapters/http.js Show resolved Hide resolved
@mbargiel mbargiel changed the title Fixing http tests to reduce timeouts when assertions fail Fixing http tests to avoid hanging when assertions fail Jan 31, 2022
@jasonsaayman jasonsaayman merged commit 9ca2779 into axios:master May 4, 2022
3 checks passed
@jasonsaayman
Copy link
Member

jasonsaayman commented May 4, 2022

Thanks for this contribution

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.

None yet

2 participants