-
Notifications
You must be signed in to change notification settings - Fork 100
Refactor render test #106
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
Refactor render test #106
Conversation
One issue with moving the render test files to the same folder as the normal tests is that the screenshot utility needs a way to identify which files it should generate reference screenshots for.
|
added render tag as well. |
@henrikrudstrom I thought that render/inspection tests are considerably different, so they should be placed in separate folders. A tag in the comment is far less visible for the developer than the path, and placing render/inspection tests in separated paths makes that more explicit and organized, imo. I am not sure if it's worth to move them to a single folder, though the Perhaps that could be solved instead this way:
or
What do you think? I would prefer to keep the Render vs (all the other) separation on the top level, though, like it is done now. |
I definetely think we should keep the category Render tests, but dont think we should restrict the use of screenshot testing to that folder, you could test for the same issue with a render test or an inspection test. Comments are not an idea solution. Folders are clearer, but we might not want to or be able to render all qml files in a folder (sub qml files etc). |
irrespective of how we organize the folders, i think it will be more flexible to to specify the tests with basic jasmine instead of globbing the folders and put some conditions based on folder names.
|
@henrikrudstrom But that would require each new render test to actually add some extra js code for it to run. That not only makes adding new tests a bit harder, but also causes changes to the same file and would require resolving conflicts in that file when some tests would be added independently in different PRs. What exactly is wrong with globbing? =) |
I think adding a line of javascript code is an acceptable effort, but its a good point about a single file that has to be edited, didnt think about that.
The advantages is see are:
|
Another idea i worked a bit on is making multiple screenshots and a function to yield execution back to the testing code (see comments). It defines 3 functions that are added to the rootObject and are also recognized by the screenshot utility.
On the test side it looks like this:
i have it halfway working, any thoughts @ChALkeR |
I don't think we should make render tests more complex. |
Hi guys! I don't 100% understand what is being discussed here. But I suggest to have 1 test subject = 1 folder. By test subject I mean group of test cases coupled together. Now we have another structure: 1 test subject = 1 script file (.js) [that's ok] + undetermined amount of qml files and other resources in some subfolder [that's confusing]. |
@pavelvasev Are you speaking about render tests or some other tests? Render tests are basically a pair of files for each test — a As for the js+qml tests, those should be probably mostly migrated to QtTest-based tests instead, see https://github.com/qmlweb/qmlweb/tree/master/tests/Auto. Btw, «Auto» (QtTest-based) tests could be categorized in any subfolders structure, if needed. |
I believe that the render tests should be kept as simple as they are now, without mising any additional glueing code or metadata to them. cb3283c adds support for running those tests using Qt QML. Also, long-term, we could replace the javascript runner for render tests with the QtTest runner (the one from cb3283c), but that would require more work on QtTest. We could throw out karma and jasmine after that. That (along with cb3283c) gives us an alternative road to refactoring tests, so I believe we can close this. Feel free to comment and/or reopen if you think that there is something else to discuss on this PR. |
Moved the render test code out of runner.js so it can be used in other test suites:
That means we can organize the tests by the subject of test instead of method of test
tests/Render
can be flagged for rendering with//@Render
as the first line of the qml file (depends on test rendering/devtools project #84) to so that qmlweb-dev knows it needs a screenshot.