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

Flaky Tests Hub #844

Open
1 of 2 tasks
doc-han opened this issue Dec 14, 2024 · 5 comments
Open
1 of 2 tasks

Flaky Tests Hub #844

doc-han opened this issue Dec 14, 2024 · 5 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@doc-han
Copy link
Collaborator

doc-han commented Dec 14, 2024

Description

This is a space dedicated to tracking known flaky tests and systematically resolving them.

Note

  1. Pick one, create issue if it doesn't exist and reference here via related issue
  2. After your issue's related PR has been merged and related issue is closed on a good note. click the checkmark!

Flaky Tests

  • "synchronously create a placeholder before generating the docs"
    Reason[1]: Promise functions start resolving as soon as they've been called. await doesn't tell a promise to start resolving but rather blocks execution only in the scope of its async function.
    Reason[2]: A placeholder is supposed to be generated quickly before the promise is resolved. Here, we're trying to test in between the time the placeholder is generator and when the promise with the actual data is resolved. Since there's no good mechanism to test in-between promise call and resolution, it makes this test very flaky. Hence, an error is thrown when the placeholder doesn't exist but we try doing JSON.parse(readFileSync(path, 'utf8'));
    test.serial(
    'synchronously create a placeholder before generating the docs',
    async (t) => {
    const path = `${DOCS_PATH}/${specifier}.json`;
    // a placeholder should not exist when we start
    const empty = await loadJSON(path);
    t.falsy(empty);
    const promise = docsHandler(options, logger, mockGen);
    // the placeholder should already be created
    const placeholder = JSON.parse(readFileSync(path, 'utf8'));
    t.truthy(placeholder);
    t.true(placeholder.loading);
    t.assert(typeof placeholder.timestamp === 'number');
    // politely wait for the promise to run
    await promise.then();
    }
    );

    Related Issue: no issue created

  • "allow a job to complete after receiving a sigterm"
    Reason: workerProcess.kill('SIGTERM') doesn't kill a process exactly after it has been called. It's async and may take some time to actually kill the process. Hence at times, the job gets processed before, other times not depending on computer resource available.
    test.serial('allow a job to complete after receiving a sigterm', (t) => {
    return new Promise(async (done) => {
    let didKill = false;
    const port = getPort();
    lightning = initLightning(port);
    const job = createJob({
    // This job needs no adaptor (no autoinstall here!) and returns state after 1 second
    adaptor: '',
    body: 'export default [(s) => new Promise((resolve) => setTimeout(() => resolve(s), 1000))]',
    });
    const attempt = createRun([], [job], []);
    lightning.once('run:complete', (evt) => {
    t.true(didKill); // Did we kill the server before this returned?
    t.is(evt.payload.reason, 'success'); // did the attempt succeed?
    t.pass('ok');
    // Give the server some time to shut down
    setTimeout(async () => {
    // The webserver should not respond
    await t.throwsAsync(() => fetch('http://localhost:2222/'), {
    message: 'fetch failed',
    });
    const finishTimeout = setTimeout(() => {
    done();
    }, 500);
    // Lightning should receive no more claims
    lightning.on('claim', () => {
    clearTimeout(finishTimeout);
    t.fail();
    done();
    });
    }, 10);
    });
    workerProcess = await spawnServer(port);
    lightning.enqueueRun(attempt);
    // give the attempt time to start, then kill the server
    setTimeout(() => {
    didKill = true;
    workerProcess.kill('SIGTERM');
    }, 100);
    });
    });

    SKIPPED
@github-project-automation github-project-automation bot moved this to New Issues in v2 Dec 14, 2024
@doc-han doc-han added good first issue Good for newcomers help wanted Extra attention is needed labels Dec 14, 2024
@josephjclark
Copy link
Collaborator

That first one is fine @doc-han - the await at the end is just to close the test out nicely. It's not really significant

@josephjclark
Copy link
Collaborator

We can skip that sigterm one, it's fine:)

@josephjclark
Copy link
Collaborator

@doc-han I'm really not sure what's up with the docgen one. It'll occasionally fail locally too, but I can't reliably repro and have yet to see the exact problem. I've spent too much time on this and I've skipped it for now.

There's clearly a timing issue go on but I can't work out where. Yes, the promise starts executing immediately. The docgen function will SYNCHRONOUSLY create a placeholder file, then asynchronously start running doc gen, and synchronously return that promise. It's like the synchronous file write hasn't actually commited to disk by the time we read the file back. Which is weird because fundamentally I expect writeFileSync('file.txt', 'x'); const f = readFileSync('file.txt', 'utf8') to reliably work. And as far as I'm concerned, that's what's going on here.

Maybe mock-fs is causing some kind of interference.

@doc-han
Copy link
Collaborator Author

doc-han commented Dec 17, 2024

@doc-han I'm really not sure what's up with the docgen one. It'll occasionally fail locally too, but I can't reliably repro and have yet to see the exact problem. I've spent too much time on this and I've skipped it for now.

Yeah, I think skipping is the best option now. The only solution is to change how docsHandler works. Instead doing two things at the same time, it could be broken into two. 1. handles placeholder generation 2. handles the actual docgen.

@josephjclark
Copy link
Collaborator

I think mock-fs on node 22 is the real problem. I think the mock is slow, or something, and it's possible to try and read the file before the write has finished in the background, and that's what's triggering these errors.

I'm certainly noticing that tests which use mock-fs are running very slowly.

I also can't seem to reproduce the uncaught error thing by throwing in strategic places. I really don't understand why the error isn't caught.

@doc-han doc-han pinned this issue Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
Status: New Issues
Development

No branches or pull requests

2 participants