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

EPIC: Improve test cases #287

Closed
prisis opened this issue Jan 24, 2022 · 17 comments
Closed

EPIC: Improve test cases #287

prisis opened this issue Jan 24, 2022 · 17 comments

Comments

@prisis
Copy link
Member

prisis commented Jan 24, 2022

List of test that need to be checked and adjusted:

test developer PR # Done
address.spec.ts @Shinigami92 #285 ✔️
all_functional.spec.ts can be skipped ✔️
animal.spec.ts @Shinigami92 #293 ✔️
commerce.spec.ts @Shinigami92 #294 ✔️
company.spec.ts @Shinigami92 #493 ✔️
database.spec.ts @Shinigami92 #393 ✔️
datatype.spec.ts @Shinigami92 #344 ✔️
date.spec.ts @Shinigami92 #312 ✔️
fake.spec.ts is okay ✔️
finance_iban.spec.ts @Shinigami92 #515 ✔️
finance.spec.ts @Shinigami92 #519 ✔️
git.spec.ts @Shinigami92 #425 ✔️
helpers.spec.ts @Shinigami92 #499 ✔️
image.spec.ts @Shinigami92 #414 ✔️
internet.spec.ts @ST-DDT, @Shinigami92 (partially) #258, #475 ✔️
locales.spec.ts can be skipped ✔️
lorem.spec.ts @Shinigami92 #473 ✔️
music.spec.ts @piotrekn #383 ✔️
name.spec.ts @Shinigami92 #372 ✔️
phone_number.spec.ts @piotrekn #396 ✔️
random.spec.ts @Shinigami92 #490 ✔️
system.spec.ts @prisis #303 ✔️
time.spec.ts @Shinigami92 #421 ✔️
unique.spec.ts @Shinigami92 #370 ✔️
vehicle.spec.ts @Shinigami92 #469 ✔️
word.spec.ts @Shinigami92 #470 ✔️
hacker.spec.ts @Shinigami92 #386 ✔️
  • First step would be to create some seeds and a seed map with return values to always create the same data.
  • Extend the test to run on every local that faker supports.
  • Remove mock calls to test the function itself and not the mocked output.
@prisis prisis added the s: pending triage Pending Triage label Jan 24, 2022
@prisis prisis added this to the v6.0.0 - Project stability milestone Jan 24, 2022
@prisis prisis added help wanted Extra attention is needed c: test and removed s: pending triage Pending Triage labels Jan 24, 2022
@github-actions
Copy link
Contributor

Hello @prisis. We like your proposal/feedback and would appreciate a contribution via a Pull Request by you or another community member. We thank you in advance for your contribution and are looking forward to reviewing it!

@Shinigami92 Shinigami92 moved this to In Progress in Faker Roadmap Jan 24, 2022
@Shinigami92 Shinigami92 self-assigned this Jan 25, 2022
@Shinigami92
Copy link
Member

Shinigami92 commented Jan 25, 2022

For the long run we could try to create a type + abstraction like this:

export type SeededRun<Module, K extends keyof Module = keyof Module> = {
  seed: number;
  expectations: {
    [P in K]: any;
  };
};

But it would be cool to also have somehow have the expectations values typed 🤔

The benefit would be to get typescript-notified if there is a missing test

@Shinigami92 Shinigami92 changed the title Improve test cases EPIC: Improve test cases Jan 26, 2022
@Shinigami92 Shinigami92 pinned this issue Jan 26, 2022
@piotrekn
Copy link
Contributor

@prisis, I can do music.spec.ts, phone_number.spec.ts and random.spec.ts. Can you assign them to me , please?

@Shinigami92
Copy link
Member

I will do 👍 Please make them in 3 dedicated PRs, then I will update the PR numbers also

@Shinigami92
Copy link
Member

@prisis you missed hacker cause hacker doesn't has a test at all 😱
I think I will target hacker as next module
I will also add it now to the list above

@prisis
Copy link
Member Author

prisis commented Jan 31, 2022

It was hacked 🤣 yeah, was printing all test file out

@piotrekn
Copy link
Contributor

piotrekn commented Feb 7, 2022

Guys, this pattern is flawed:

  // Create and log-back the seed for debug purposes
  faker.seed(Math.ceil(Math.random() * 1_000_000_000));

  describe(`random seeded tests for seed ${faker.seedValue}`, () => {

It should be more like this:

  // Create and log-back the seed for debug purposes
  const seed = Math.ceil(Math.random() * 1_000_000_000);

  describe(`random seeded tests for seed ${seed}`, () => {
    beforeAll(() => {
      faker.seed(seed);
    });

...or even beforeEach instead of beforeAll.
The problem is that faker.seedValue is a global variable, and every faker.seed() changes it. Once seededRuns have executed, the last seed set remains, not a random one we expect. The expression faker.seed(Math.ceil(Math.random() * 1_000_000_000)); is executed once before all tests, on declaration.

Please review your PRs for that case.

@Shinigami92
Copy link
Member

Shinigami92 commented Feb 8, 2022

Guys, this pattern is flawed:

  // Create and log-back the seed for debug purposes
  faker.seed(Math.ceil(Math.random() * 1_000_000_000));

  describe(`random seeded tests for seed ${faker.seedValue}`, () => {

It should be more like this:

  // Create and log-back the seed for debug purposes
  const seed = Math.ceil(Math.random() * 1_000_000_000);

  describe(`random seeded tests for seed ${seed}`, () => {
    beforeAll(() => {
      faker.seed(seed);
    });

...or even beforeEach instead of beforeAll. The problem is that faker.seedValue is a global variable, and every faker.seed() changes it. Once seededRuns have executed, the last seed set remains, not a random one we expect. The expression faker.seed(Math.ceil(Math.random() * 1_000_000_000)); is executed once before all tests, on declaration.

Please review your PRs for that case.

You are missing that we added faker.seed(seed) in every single it block to ensure this behavior.
Even beforeEach was not safe enough, cause we sometimes have nested describe+it blocks in this loop.
If you found a specific test were we overseen this, please report it to us.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 8, 2022

@Shinigami92 I think you and @piotrekn are referring to different things.

As you can see there is no faker.seed() inside this section anymore:

faker/test/animal.spec.ts

Lines 101 to 115 in f1884be

// Create and log-back the seed for debug purposes
faker.seed(Math.ceil(Math.random() * 1_000_000_000));
describe(`random seeded tests for seed ${faker.seedValue}`, () => {
for (let i = 1; i <= NON_SEEDED_BASED_RUN; i++) {
for (const functionName of functionNames) {
describe(`${functionName}()`, () => {
it(`should return random value from ${functionName} array`, () => {
const actual = faker.animal[functionName]();
expect(faker.definitions.animal[functionName]).toContain(actual);
});
});
}
}
});

However, this isn't needed because the main purpose of those methods is to randomly execute the methods and check for unexpected behavior. These tests should be removed or disabled by default in the medium term to remove the flaky test coverage behavior.
The faker seed is set once and logged to ensure that in the event of a failure, the test setup can be recovered by manually setting the seed. It is not required to have the exact seed for all methods.

@Shinigami92
Copy link
Member

Shinigami92 commented Feb 8, 2022

@ST-DDT ah yes, you are totally right

The weather is grey here and I feel a bit weak today, so sorry if I confuse some stuff

@Shinigami92
Copy link
Member

Vehicle is driving me crazy, so I will tackle that now 😜

@Shinigami92
Copy link
Member

Shinigami92 commented Feb 11, 2022

For some tests I have no words...

@Shinigami92
Copy link
Member

Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

@Shinigami92
Copy link
Member

I will now rewrite the internet

@Shinigami92
Copy link
Member

I will try targeting finance_iban.spec.ts now

@Shinigami92
Copy link
Member

We are near to finish 🏁

Only reviews are needed!

@Shinigami92 Shinigami92 moved this from In Progress to Awaiting Review in Faker Roadmap Feb 19, 2022
@Shinigami92 Shinigami92 removed the help wanted Extra attention is needed label Feb 19, 2022
@Shinigami92
Copy link
Member

All done 🎉

Repository owner moved this from Awaiting Review to Done in Faker Roadmap Feb 19, 2022
@Shinigami92 Shinigami92 unpinned this issue Feb 19, 2022
@ST-DDT ST-DDT removed this from Faker Roadmap Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants