add test for accidental replace in v10.8.1 #1571
Conversation
|
Verified, tests fail here, but succeed on master, merging, thanks! |
| import {renderToString} from 'react-dom/server' | ||
| import expect from 'expect.js' | ||
| import {stripIndent} from 'common-tags' | ||
| import createCommonBaseTests from '../test-utils/createCommonBaseTests' | ||
| import {getCss, getStyle, removeWhitespace, resetSheets} from '../../../tests/utils' |
There was a problem hiding this comment.
Oh didn't notice it before merging, something.test.js are supposed to be unit tests only, so no dom dependency
@seiyab can you please move dom/cssom specific stuff to react-jss/tests?
Do we actually need DOM-based assertions for this test or would be registry.toString() already enough?
There was a problem hiding this comment.
OH I just checked, we don't do DOM based assertions in react-jss at all, because we wanted to keep it react specific without going low-level.
So if you want to assert CSSOM, we need we would need a low level test for this behavior in jss core, if that doesn't exist already.
There was a problem hiding this comment.
can you please move dom/cssom specific stuff to react-jss/tests?
OK, I will.
Do we actually need DOM-based assertions for this test or would be registry.toString() already enough?
If I remember correctly, we need it.
registry.toString() rendered correct string, but not in DOM.
There was a problem hiding this comment.
If lower level behavior is not new to jss, so all we test is react-jss logic - we don't need to assert CSSOM here
There was a problem hiding this comment.
All right.
I would move it to jss core.
There was a problem hiding this comment.
btw
Tests that focus on DomRenderer are unnatural because it doesn't fully work without StyleSheet
We use stylesheet for the most tests
There was a problem hiding this comment.
Testing DomRenderer.replaceRule via StyleSheet and Rule is impossible when setSelector works.
if setSelector was an issue in react-jss, then we just need to log what react-jss passes to jss and do the same thing in jss
There was a problem hiding this comment.
In fact the ideal functional jss test looks to me like this:
createStyleSheet(styles).attach()
expect(getCss()).to.be(....)There was a problem hiding this comment.
The cause of #1565 (comment) is DomRenderer.refCssRule with index < cssRule.length - 1 , that exists from more than a year ago.
However, jss <= 10.8.0 rarely goes the path. Because DomRenderer.replaceRule is only for some old browsers.
So it is difficult to test it at v10.8.0 or v10.8.2.
On the other hand, v10.8.1 waked the bug. It utilized DomRenderer.replaceRule in StyleSheet.replaceRule and replaced rule by different rule, that caused #1565 (comment) .
This react-jss test checks "rerendering component should not delete rule by update of another rule".
v10.8.0 passes the test because v10.8.0 just adds more and more rules on update rather than replace.
v10.8.1 fails the test because v10.8.1 accidentally replace rules by other rules in DOM though it replaces rules in RuleList correctly.
There was a problem hiding this comment.
I'm going to write tests in PR that also adds replaceRule, so that we can write test for DomRenderer.refCssRule and DomRenderer.replaceRule.
Please correct me if I'm going wrong way.
Corresponding Issue(s):
Issue: #1565
What Would You Like to Add/Fix?
This commit itself doesn't add any features / fix any problems
Todo
Expectations on Changes
Test will fail unless #1565 is fixed
Changelog