Skip to content

removed incorrect test #1474

Closed
wants to merge 1 commit into from

4 participants

@matthewmueller

this test could be valid, but it uses the ok(result, msg) signature incorrectly. I did notice a discrepancy with this test though.

Safari 7 returns "auto", while Chrome 31 returns "-16px".

@matthewmueller matthewmueller removed incorrect test
this test could be valid, but it uses the `ok(result, msg)` signature incorrectly. I did notice a discrepancy with this test though.

Safari 7 returns "auto", while Chrome 31 returns "-16px".
23636a5
@markelog
jQuery Foundation member
markelog commented Jan 3, 2014

This is indeed incorrect signature for ok function, Safari 7 returns "auto" here because this block does not have position: absolute, so setting top property would not affect it.

This assertion is required only for oldIE, so it can be removed from master branch in 1.x-master however, it should be fixed.

Plus associated regexp for master branch could be simplified, but it's probably not worth it.

@MatthewMueller would you like to take care of that?

@matthewmueller

hmm, i'm not sure i follow what changes need to be made.

@markelog
jQuery Foundation member
markelog commented Jan 3, 2014

@MatthewMueller we should leave this test in for 1.x-master (which is supports IE6-8) but changed it, so it would actually test stuff, i'm asking if you want to also submit PR for that branch.

And if you feel tough today :-), you can check out associated regxep in this branch that could be simplified, but that's really optional.

@markyun
markyun commented Jan 6, 2014

good

@dmethvin
jQuery Foundation member

@MatthewMueller I think @markelog was looking at each thing in both the unit tests and css.js module and realizing that they could all use some work. Think of it like one of those home shows where they go in to replace a light bulb and end up gutting the bathroom.

I think we all agree this test isn't right. How about if we remove it and open a ticket for making the .css() unit tests better? @markelog are there other specific things you want to test? It seems like we have very little in the unit tests to check set/get negative margins or positions.

@dmethvin
jQuery Foundation member
dmethvin commented Dec 9, 2014

Alright, let's wrap this one up.

Tracing this back and just for documentation's sake, the patch introducing this test was for #3331 to fix the regex to allow negative values for CSS properties. As mentioned above, the current test doesn't really do anything. In a quick scan I don't see any tests that used negative margin (px or em) which seems bad. I'm going to land this in both master and compat and open a ticket to have at least a few tests that involve negative numbers.

@dmethvin dmethvin added a commit that referenced this pull request Dec 9, 2014
@dmethvin dmethvin Css: Remove non-functional unit test for negative margin
Thanks @MatthewMueller

Closes gh-1474
Ref gh-1918
(cherry picked from commit 30b3b33251abc38386b787c64494265177302b86)
1ece10f
@dmethvin dmethvin closed this in 4ab7431 Dec 9, 2014
@dmethvin
jQuery Foundation member
dmethvin commented Dec 9, 2014

Hi @MatthewMueller, really sorry about the long delay on this, totally my fault for letting it sit around for so long. It looks like you hadn't signed our CLA. To save time and end the suffering I landed this myself since it was just a deletion. If you'd like to submit patches, please do sign http://contribute.jquery.org/CLA/ , I should have checked for it earlier. We're hoping to get some automated processes set up soon to check this as soon as the PR appears.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.