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

Test coverage timing issue #16

Closed
hueniverse opened this issue Sep 9, 2015 · 11 comments
Closed

Test coverage timing issue #16

hueniverse opened this issue Sep 9, 2015 · 11 comments
Labels
Milestone

Comments

@hueniverse
Copy link
Member

@hueniverse hueniverse commented Sep 9, 2015

Some lines are not always covered. Must be a timing issue with the tests.

59 tests complete
Test duration: 216 ms
Assertions count: 157 (verbosity: 2.66)
No global variable leaks detected
Coverage: 98.16% (6/326)
lib/index.js missing coverage on line(s): 365, 366, 368, 418, 419, 420
Code coverage below threshold: 98.16 < 100
Linting results: No issues
@hueniverse hueniverse added the test label Sep 9, 2015
@johnbrett

This comment has been minimized.

Copy link
Contributor

@johnbrett johnbrett commented Sep 10, 2015

This is down to the sudo: false flag on travis. Investigating.

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Sep 10, 2015

This happens on my local machine every 4-5 tries.

@johnbrett

This comment has been minimized.

Copy link
Contributor

@johnbrett johnbrett commented Sep 11, 2015

This is proving quite difficult to debug. It seems that Pez isn't always sending the 'close' event, or at least not in the right order of events.

The following tests give inconsistent results over the course of a few runs:

   it('parses a multipart file as file', function (done) {

        var path = Path.join(__dirname, './file/image.jpg');
        var stats = Fs.statSync(path);

        var form = new FormData();
        form.append('my_file', Fs.createReadStream(path));
        form.headers = form.getHeaders();

        Subtext.parse(form, null, { parse: true, output: 'file' }, function (err, parsed) {

            expect(err).to.not.exist();

            expect(parsed.payload.my_file.bytes).to.equal(stats.size);

            var sourceContents = Fs.readFileSync(path);
            var receivedContents = Fs.readFileSync(parsed.payload.my_file.path);
            Fs.unlinkSync(parsed.payload.my_file.path);
            expect(sourceContents).to.deep.equal(receivedContents);
            done();
        });
    });

    it('parses multiple files as files', function (done) {

        var path = Path.join(__dirname, './file/image.jpg');
        var stats = Fs.statSync(path);

        var form = new FormData();
        form.append('file1', Fs.createReadStream(path));
        form.append('file2', Fs.createReadStream(path));
        form.headers = form.getHeaders();

        Subtext.parse(form, null, { parse: true, output: 'file' }, function (err, parsed) {

            expect(err).to.not.exist();
            expect(parsed.payload.file1.bytes).to.equal(stats.size);
            expect(parsed.payload.file2.bytes).to.equal(stats.size);
            Fs.unlinkSync(parsed.payload.file1.path);
            Fs.unlinkSync(parsed.payload.file2.path);
            done();
        });
    });

Still investigating.

@johnbrett

This comment has been minimized.

Copy link
Contributor

@johnbrett johnbrett commented Sep 13, 2015

Ok there's a race condition causing the code coverage not to be 100%, there's no bugs to worry about, but it means execution order cannot be guaranteed making 100% code coverage pretty difficult.

Problem is in the lines below:

var onPart = function (part) {

    if (self.settings.output === 'file') { // Output: 'file'
        var id = nextId++;
        pendingFiles[id] = true;
        self.writeFile(part, function (err, path, bytes) {

            delete pendingFiles[id]; // <- sometimes called before or after the 'onClose' event
            // ... rest of 'onPart' ...
}

var onClose = function () {

    if (Object.keys(pendingFiles).length) { // <- 0 or 1 depending on whether onClose event fired before writeFile finished
        closed = true;
        return;
    }

    // ... rest of 'onClose ..
}

Meaning for two possible flows A:

  • 'onPart' event
  • pendingFiles[id] = true;
  • self.writeFile(part, function (err, path, bytes) {
  • delete pendingFiles['id'];
  • 'onClose' event
  • if (Object.keys(pendingFiles).length) { // 0

B where sometimes the 'onClose' event fires before the self.writeFile callback is called, meaning:

  • 'onPart' event
  • pendingFiles[id] = true;
  • self.writeFile(part, function (err, path, bytes) {
  • 'onClose' event
  • if (Object.keys(pendingFiles).length) { ... } // This now becomes 1
  • delete pendingFiles['id'];

It seems the problematic test is 51: 'parses multiple files as files'. Sometimes it goes by flow A, making those lines in the travis test case not be covered. @hueniverse is there a way I can force event ordering here or do you know of a better approach?

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Sep 14, 2015

Will different files sizes help? Maybe monkey patch some of the file APIs to make them produce different timing?

@johnbrett

This comment has been minimized.

Copy link
Contributor

@johnbrett johnbrett commented Sep 19, 2015

Added different file sizes & combinations to tests in #17, now getting 100% coverage.

@johnbrett johnbrett closed this Sep 19, 2015
@johnbrett

This comment has been minimized.

Copy link
Contributor

@johnbrett johnbrett commented Sep 19, 2015

Out of interest - is it possible to monkey patch the file APIs without modify lib/index.js? I can't see any way to access the internals because of the way we create the object.

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Sep 19, 2015

Which file API?

@johnbrett

This comment has been minimized.

Copy link
Contributor

@johnbrett johnbrett commented Sep 19, 2015

either:
internals.Parser.prototype.writeFile https://github.com/hapijs/subtext/blob/master/lib/index.js#L432

or the Fs.unlink inside writeFile here: https://github.com/hapijs/subtext/blob/master/lib/index.js#L451

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Sep 19, 2015

You can easily monkey patch Fs methods. Just make sure to set the test mode to {parallel:false} and to put things back together before the test is done.

@johnbrett

This comment has been minimized.

Copy link
Contributor

@johnbrett johnbrett commented Sep 20, 2015

TIL how to monkey patch Fs methods, thanks :)

@johnbrett johnbrett added this to the 2.0.1 milestone Oct 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.