Some tests seem to fail due to time constraints

[dkastl]

It seems some failed tests were caused by execution time constraints.

After increasing the limits, the tests passed.

 FAIL  __tests__/generate.test.ts (19.037 s)
  ● generate integrate test › use fontSubset template › sample

    thrown: "Exceeded timeout of 10000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      103 |
      104 |   describe('use fontSubset template', () => {
    > 105 |     test(`sample`, async () => {
          |     ^
      106 |       const inputs = [{ field1: 'SauceHanSansJP', field2: 'SauceHanSerifJP' }];
      107 |       const template: Template = {
      108 |         basePdf: BLANK_PDF,

      at __tests__/generate.test.ts:105:5
      at __tests__/generate.test.ts:104:3
      at Object.<anonymous> (__tests__/generate.test.ts:6:1)

Setting the timeout value to 300000 made the test pass.

 FAIL  __tests__/integration2.test.ts (21.024 s)
  ● generate integration test(barcode, business) › {
  'Aone31555QRコード': [Object],
  'Aone31553QRコード': [Object],
  'Aone72230JANコード短縮': [Object],
  'Aone72230JANコード標準': [Object],
  barcodes: [Object]
} › snapshot Aone31555QRコード

    expect(received).toBeLessThan(expected)

    Expected: < 2
    Received:   2.15055569

      33 |         const hrend = process.hrtime(hrstart);
      34 |         const execSeconds = hrend[0] + hrend[1] / 1000000000;
    > 35 |         expect(execSeconds).toBeLessThan(2);
         |                             ^
      36 |
      37 |         const tmpFile = getPdfTmpPath(`${key}.pdf`);
      38 |         const assertFile = getPdfAssertPath(`${key}.pdf`);

      at Object.<anonymous> (__tests__/integration2.test.ts:35:29)

Increasing the limit from 2 -> 3 seconds made the test pass.

And similar in the 3rd case adding a second solved the problem.

  ● generate integration test(barcode, business) › {
  'Aone31555QRコード': [Object],
  'Aone31553QRコード': [Object],
  'Aone72230JANコード短縮': [Object],
  'Aone72230JANコード標準': [Object],
  barcodes: [Object]
} › snapshot Aone31553QRコード

    expect(received).toBeLessThan(expected)

    Expected: < 2
    Received:   2.452473399

      33 |         const hrend = process.hrtime(hrstart);
      34 |         const execSeconds = hrend[0] + hrend[1] / 1000000000;
    > 35 |         expect(execSeconds).toBeLessThan(2);
         |                             ^
      36 |
      37 |         const tmpFile = getPdfTmpPath(`${key}.pdf`);
      38 |         const assertFile = getPdfAssertPath(`${key}.pdf`);

      at Object.<anonymous> (__tests__/integration2.test.ts:35:29)

So I was wondering, are these constraints more or less arbitrary or based on experience?

[peteward]

I think they are mainly based on experience of how long it takes to run on the github actions.

The intention is to spot changes that reduce efficiency in the processing time of the generator.

For me, these tests run much faster than this on a Macbook pro.

[dkastl]

I see, this makes sense.
And I think it's a good idea to watch out for performance issues.

That said, it happened for the first time I ran into these errors, so eventually something got slower recently. I wouldn't say my Thinkpad X1 is a slow PC, so I expect others to see these errors as well, if they run the tests.
Obviously I could just run the tests and ignore the errors, but that doesn't feel like a good thing either.

How about not failing a test but provide a warning?
Here is an idea: dkastl@82ac920
Obviously this won't work if the goal is to fail the test in Github Actions.

Beside that, I'm trying to understand the timeout settings in this test (@hand-dot):
https://github.com/pdfme/pdfme/blob/main/packages/generator/__tests__/generate.test.ts#L105-L153

If I'm not wrong, then jest.setTimeout(30000); sets the timout for all tests in a file, but the 10000 at the end of the test function actually sets the timeout for this function to 10s. So the increase to 30s does not have any effect in this function.

I assume it should be

test(`sample`, async () => {
  // ... test implementation ...
}, 30000);

This test also fails for me in rare cases. It seems when I run it for the first time. So settins the the timeout to 30s would defnitely help.

[hand-dot]

Hmm, I want to focus solely on whether it succeeds or fails on CI.
The speed of creating PDFs can vary depending on machine power, Node.js version, OS, and other factors.

If we focus only on the CI environment, there's no need to consider these. Even if the tests fail on your machine, I'll merge the PR as long as it succeeds on CI.

[dkastl]

Well, if the tests are mainly intended for CI, then I would not change to issue warnings, because then CI will not fail anymore.

Let me think if there is another workaround, because I find it convenient to run the tests. I just don't want to adjust the timeouts every time ;-)

[dkastl]

How about the following?
https://github.com/pdfme/pdfme/compare/main...dkastl:pdfme:ftr/performance-warning?expand=1

The GH Actions test workflow sets the environment variable CI to true: https://github.com/pdfme/pdfme/blob/main/.github/workflows/test.yml#L23
So it is possible to fail tests with error in in GH Actions, otherwise provide a warning. And the threshold value could be also set as environment variable.

Would this solve it for all relevant cases? Or too complicated?

[hand-dot]

@dkastl Perfect, Please submit your PR