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

Update qunit and sinon #2338

Merged
merged 5 commits into from May 11, 2016

Conversation

Projects
None yet
5 participants
@guerler
Copy link
Contributor

commented May 10, 2016

No description provided.

@guerler guerler added this to the 16.07 milestone May 10, 2016

@guerler guerler force-pushed the guerler:update_qunit_000 branch 5 times, most recently from 60abd02 to 192a8b6 May 10, 2016

@guerler guerler force-pushed the guerler:update_qunit_000 branch from f2df873 to 7e63ae6 May 11, 2016

@guerler guerler force-pushed the guerler:update_qunit_000 branch from 62fd618 to b7dc5cd May 11, 2016

dannon and others added some commits May 11, 2016

@guerler guerler added status/review and removed status/WIP labels May 11, 2016

server.respond();
deepEqual( requestJSON, DATA._1requestJSON );
});

This comment has been minimized.

Copy link
@carlfeberhard

carlfeberhard May 11, 2016

Contributor

I don't understand. The only change here is moving the test? What was broken?

This comment has been minimized.

Copy link
@guerler

guerler May 11, 2016

Author Contributor

Underscore's _.uniqueId function is used to index the datasets in the collection. I noticed that all unique Ids are shifted by 1 when the test is reloaded in the browser e.g. Safari. As a consequence the test only passes every second time. You can try this out by shifting the test back to the end, opening it in the browser and then subsequently reloading the browser window. I am not entirely sure why this is happening, but moving the test to the beginning of the test file fixed the issue reproducibly.

This comment has been minimized.

Copy link
@carlfeberhard

carlfeberhard May 11, 2016

Contributor

I can't get the error back using the method you mention (revert the change + safari). Even on a second reload.

This feels like we're masking a different error and we should probably find the actual cause/reason, but it's just moving the test around so NBD I guess. Just curious what was going on here.

This comment has been minimized.

Copy link
@jmchilton

jmchilton May 11, 2016

Member

@carlfeberhard Is DATA._1 simulating a Galaxy API call? Should the datasets have explicit IDs?

This comment has been minimized.

This comment has been minimized.

Copy link
@guerler

guerler May 11, 2016

Author Contributor

I understand, I am also curious why exactly this is happening. Thanks for looking into it and thanks for the help while updating the libraries. Can you reproduce the above failing test result (https://travis-ci.org/galaxyproject/galaxy/jobs/129318747) locally by running the test in the console?

This comment has been minimized.

Copy link
@guerler

guerler May 11, 2016

Author Contributor

@galaxybot test this

This comment has been minimized.

Copy link
@guerler

guerler May 11, 2016

Author Contributor

Restarting test because of unrelated/transiently failing api timeout test.

This comment has been minimized.

Copy link
@carlfeberhard

carlfeberhard May 11, 2016

Contributor

It does fail the test every time when using: grunt --test=list-of-pairs-collection-creator.html

The ids are generated in _ensureIds in list-of-pairs-collection-creator. Generally it's never hit because, like the test data here (edit: _un_like the test data here), the datasets used already have ids. It was meant for datasets that aren't even in galaxy yet - i.e. they were planned so we could generate collections from the upload form.

I'm not entirely sure why (or if) that's what's causing the error here. I'll check it out - thanks

This comment has been minimized.

Copy link
@carlfeberhard

carlfeberhard May 11, 2016

Contributor

So:

  • qunit may have reversed the order it's tests are run - it now runs like a queue and it used to run like a stack. That's why the last test evaluated first.
  • It started at 2 (_.uniqueId generally starts at 1), because _.uniqueId is also used by backbone for it's cid's.
  • The ids for DATA._1 were only generated once (before on test 6, now on test 2 - 1 doesn't use them) because _ensureIds would decorate the DATA._1 objects directly. All tests after the first wouldn't need ids - they were already there.

Because the order changed:

  • test 1 ran first and _.uniqueId was used for the cid of the creator in test 1 (id -> 1)
  • test 2 then ran and _.uniqueId was used for the cid of the creator in test 2 (id -> 2)
  • ids were generated for ensureIds starting at the next _.uniqueId: 3
    --> the actual ids were now starting at 3 changed and didn't match up with the expected.

The main issue is that this is just crappy test design on my part. Order shouldn't matter, so It shouldn't expect those ids if it's going to use the global counter in uniqueId - I had also forgotten that backbone uses that counter, too. And crappy code in the _ensureIds shouldn't be decorating the objects directly, but cloning them first.

In any event, like I said above - I'm fine with the moving that to the first test, I just wanted to find out why. Feel free to leave as is, or alternately change datasets1 in paired-collection-creator.data.js to:

var datasets1 = [
    { name: 'SET1-01_1.fastq', id: '14' },
    { name: 'SET1-01_2.fastq', id: '15' },
    { name: 'SET1-02_1.fastq', id: '12' },
    { name: 'SET1-02_2.fastq', id: '13' },
    { name: 'SET1-03_1.fastq', id: '10' },
    { name: 'SET1-03_2.fastq', id: '11' },
    { name: 'SET1-04_1.fastq', id: '8' },
    { name: 'SET1-04_2.fastq', id: '9' },
    { name: 'SET1-05_1.fastq', id: '6' },
    { name: 'SET1-05_2.fastq', id: '7' },
    { name: 'SET1-06_1.fastq', id: '4' },
    { name: 'SET1-06_2.fastq', id: '5' },
    { name: 'SET1-07_1.fastq', id: '2' },
    { name: 'SET1-07_2.fastq', id: '3' }
];

This will prevent the ids being generated at all.

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2016

Thanks for looking into it. I am fine with leaving it as it is too.

@dannon dannon merged commit 09e8f0a into galaxyproject:dev May 11, 2016

3 of 4 checks passed

api test Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished.
Details
toolshed test Build finished.
Details

@galaxyproject galaxyproject deleted a comment from galaxybot Jun 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.