-
Notifications
You must be signed in to change notification settings - Fork 2
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
improving testing on marionette/backbone #29
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left feedback. I don't think this should live here in our public javascript repo, but should instead move to one of our private repos.
@@ -476,15 +476,34 @@ We have extended Sinon with a few custom functions, which you can see in django/ | |||
|
|||
### Running the Jasmine Tests | |||
|
|||
Go to <https://www.evbdev.com/js-tests> to see the index of the JS unit test suites. Most of the modern unit tests are under require at <https://www.evbdev.com/js-tests/require>. | |||
We have several system cohexisting in our testing environment and depending on our runner version is how we run them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May reword to say:
We have several systems that coexist in our testing environment, and the version of the test runner determines how the tests are executed.
|:--------------------------------|:--------------:|--------------------------------------------------------| | ||
| `mako`, `js`, global name space |1.2.0 | `tug attach core-web && test_eb ebapps.js_tests.tests`| | ||
| `handlebars`, `js` |1.2.0, 2.5.0 | `tug attach core-frontend && npm run tests` | | ||
| `react` + `jsx` + `es6` |2.5.0 | `tug attach core-frontend && npm run tests` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be:
react
,jsx
,es6
(separate by commas instead of +)
### known issues | ||
|
||
####rendering on the Dom | ||
jasmine comes with a handy `setFixture()` function that attaches html to the runner so we can render there and try our code. this results often in issues with what we leave behind or conflicts with the actual runner report. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jasmine comes with a handy
setFixture
function that attaches HTML to the runner so that we can render there and test our code. This often results leaks in the DOM or conflicts with the actual runner report.
####rendering on the Dom | ||
jasmine comes with a handy `setFixture()` function that attaches html to the runner so we can render there and try our code. this results often in issues with what we leave behind or conflicts with the actual runner report. | ||
|
||
solution: it most cases the solution for us is just to create a fake node (not attached to the dom) ex: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"in" instead of "it"
jasmine comes with a handy `setFixture()` function that attaches html to the runner so we can render there and try our code. this results often in issues with what we leave behind or conflicts with the actual runner report. | ||
|
||
solution: it most cases the solution for us is just to create a fake node (not attached to the dom) ex: | ||
`new Marionette.ItemView(el: $(<div id=fake ></div>)`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put this in a code block instead of inline code
`new Marionette.ItemView(el: $(<div id=fake ></div>)`) | ||
|
||
####avoiding toBeVisible | ||
Jquery does several tricky things to find out if an element is hidden or not. assuming that an element is attached to the dom. it also leads to confusing tests, giving that not always the element we are testing gets hidden by direct result of our test case rather is produced by the parent element or css generically applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the best rewrite I could come up with:
jQuery does several tricky things to find out whether or not an element is hidden (assuming that an element is attached to the DOM). This means that a test could successfully verify that an element is hidden even if the test itself never hid the element. This is because the test made an ancestor element hidden, so the child element also became hidden. Or worse, a previous test hid the ancestor element and the current test is implicitly relying on that element being hidden. Avoid
toBeVisible
helps prevent this weird test cases.
100eb20
to
d910dc6
Compare
Should we close this PR? |
WIP, but the info is there.
the table looks like that