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

refactor tests to use fetch instead of request #1016

Closed
thescientist13 opened this issue Dec 2, 2022 · 4 comments · Fixed by #1172
Closed

refactor tests to use fetch instead of request #1016

thescientist13 opened this issue Dec 2, 2022 · 4 comments · Fixed by #1172
Assignees
Labels
chore unit testing, maintenance, etc good first issue Good for newcomers v0.29.0

Comments

@thescientist13
Copy link
Member

thescientist13 commented Dec 2, 2022

As part of the #1014, I realized that we although we have been using request to run HTTP calls in our tests, now with native fetch, we should use that instead. That, and also, request is deprecated. 😅

So for example, in a few specs we do something like this when testing a server based command

describe('Serve command with API specific behaviors for a JSON API', function() {
  let response = {};

  before(async function() {
    return new Promise((resolve, reject) => {
      request.get(`${hostname}/api/greeting?name=${name}`, (err, res, body) => {
        if (err) {
          reject();
        }

        response.body = JSON.parse(body);

        resolve();
      });
    });
  });

  ...
})

Instead, we should now be able to do this

describe('Serve command with API specific behaviors for a JSON API', function() {
  const name = 'Greenwood';
  let response = {};
  let data = {};
  
  before(async function() {
    response = await fetch(`${hostname}/api/greeting?name=${name}`);
    data = await response.json();
  });
  
  ...
})
@thescientist13 thescientist13 added good first issue Good for newcomers chore unit testing, maintenance, etc labels Dec 2, 2022
@thescientist13 thescientist13 changed the title refactor test to use fetch instead of request refactor tests to use fetch instead of request Dec 2, 2022
@thescientist13 thescientist13 mentioned this issue Dec 10, 2022
22 tasks
@DevLab2425
Copy link
Contributor

@thescientist13

I gave this one a shot, but getting some failures when running the tests on master.

Steps to reproduce:

  1. Clone / pull latest from master
  2. yarn install --force to ensure all deps are installed
  3. yarn test...

The coverage report is created, but the tests fail along with the overall process. Am I missing something?

2587 passing (4m)
27 pending
31 failing
...
error Command failed with exit code 31.

@thescientist13
Copy link
Member Author

thescientist13 commented Oct 18, 2023

@DevLab2425
So this is just on master without making any changes to the code? That's odd, I just ran them now myself and all seems fine?

  2677 passing (3m)
  27 pending

yarn install --force to ensure all deps are installed

Does this change the lockfile at all? I wonder what version of Yarn you are using? Just yarn install in theory should be enough. (I'm still on 1.x)

One thought is for one of the failing test cases, find it's corresponding *.spec.js file and

  1. Change to describe.only at the top
  2. Pass true to runner = new Runner() to enable logging output

Be curious to see what errors you are getting. 👀

@DevLab2425
Copy link
Contributor

@thescientist13
I was able to track it down using the true parameter. It looks like I had a process hanging on port 8080 and it was causing the runner to fail with an "in use" error. 🤦

Killed that process and things are running fine. Not sure how I missed that otherwise. Sorry for the false alarm.

@thescientist13
Copy link
Member Author

All good! I'd be lying if I said I hadn't done that myself 😇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore unit testing, maintenance, etc good first issue Good for newcomers v0.29.0
Projects
Development

Successfully merging a pull request may close this issue.

2 participants