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

Add tests certifying offset isn't broken after a recent revert #2909

Closed
mgol opened this issue Feb 8, 2016 · 8 comments
Closed

Add tests certifying offset isn't broken after a recent revert #2909

mgol opened this issue Feb 8, 2016 · 8 comments
Assignees
Labels
Milestone

Comments

@mgol
Copy link
Member

mgol commented Feb 8, 2016

Basically, we need tests certifying #2828 & #2836 work after the revert done in 49833f7. We should land them on all 3 branches.

@mgol mgol added this to the 1.12.1/2.2.1 milestone Feb 8, 2016
mgol referenced this issue Feb 8, 2016
This reverts commit 2d71594.

This commit provoked new issues: gh-2836, gh-2828.

At the meeting, we decided to revert offending commit
(in all three branches - 2.2-stable, 1.12-stable and master)
and tackle this issue in 3.x.

Fixes gh-2828
@markelog
Copy link
Member

We should land them on all 3 branches.

mm, doesn't seem worth it, but okay

markelog added a commit that referenced this issue Feb 11, 2016
markelog added a commit that referenced this issue Feb 11, 2016
@mgol
Copy link
Member Author

mgol commented Feb 12, 2016

Thanks. :)

@mgol
Copy link
Member Author

mgol commented Feb 13, 2016

@markelog Those tests are broken on all browsers, both on 2.2-stable and on 1.12-stable. Could you fix? I'll reopen for now.

@mgol mgol reopened this Feb 13, 2016
@mgol
Copy link
Member Author

mgol commented Feb 13, 2016

This seems to be caused by using $ instead of jQuery. I'm surprised it works on master, is this test using a correct jQuery version?

@mgol
Copy link
Member Author

mgol commented Feb 13, 2016

Also, I've noticed this only now but the variable to which you assign assert.async() should be called done, not stop, to better align with other tests (in the previous nomenclature it would be called start, not stop so it was different than usual either way).

@markelog
Copy link
Member

For some reason that does work with amd, that's reason i missed it.

should be called done, not stop

I think it is fine as is.

markelog added a commit that referenced this issue Feb 13, 2016
For some reason that works with `amd` but not with builded version

Fixes gh-2909
markelog added a commit that referenced this issue Feb 13, 2016
For some reason that works with `amd` but not with builded version

Fixes gh-2909
@mgol
Copy link
Member Author

mgol commented Feb 13, 2016

I think it is fine as is.

It's not stopping anything, it just notifies the test runner it's finished. It was called start in the old API as it was resuming the test suite (after calling stop() before).

It's also almost universally called done, both by other test runners and in our code that has switched to the new API. This is now the only place in the whole suite where it's called stop.

@markelog
Copy link
Member

it just notifies the test runner it's finished

And that whole process is called stopping, whereas "stop" is synonym of "finish".

I think we are free to choice the name of our variables as long as those names sufficiently reflects their purpose.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

2 participants