Skip to content

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

Closed
wants to merge 5 commits into from
Closed

Refactor render test #106

wants to merge 5 commits into from

Conversation

henrikrudstrom
Copy link
Member

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

  var itCanRender = prefixedRenderTester("QtQuick/qml/Rectangle");
  itCanRender("Color");
  • Moved some of the render tests to their respective QtQuick test suite...
  • Files that are not in 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.

@henrikrudstrom
Copy link
Member Author

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.
One way could be to flag them with a comment, and possibly add parameters to the screenshot utility should that be needed.

//Render, {overwrite: false, states: ["", "pressed"] }
Item{ ...

@henrikrudstrom
Copy link
Member Author

added render tag as well.
@igosoft @ChALkeR @pavelvasev would be nice would be nice if someone could test/approve/object to this

@ChALkeR
Copy link
Member

ChALkeR commented Mar 6, 2016

@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 Simple render tests could perhaps be instead split per-component inside the Render folder. The idea behind the Simple path was that sometime we might introduce Complex render tests that would show a huge scene and test a lot of things at once and how they work together.

Perhaps that could be solved instead this way:

Render/QtQuick/RectangleSomething.qml
Render/QtQuick/RectangleSomething.png
Render/Complex/HugeSceneA.qml
Render/Complex/HugeSceneB.qml

or

Render/QtQuick/Rectangle/Something.qml
Render/QtQuick/Rectangle/Something.png
Render/Complex/HugeSceneA.qml
Render/Complex/HugeSceneB.qml

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.

@henrikrudstrom
Copy link
Member Author

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).
perhaps the screenshot function in the test code should report missing pngs to a file. then we use that file to generate reference screenshots?

@ChALkeR ChALkeR added the tests label Mar 6, 2016
@henrikrudstrom
Copy link
Member Author

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.

describe("render tests", function(){
  renderTest("RectangleColor")
  renderAsyncTest("Image")
  it('can render my very special case', function(){
    ....

@ChALkeR
Copy link
Member

ChALkeR commented Mar 6, 2016

@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? =)

@henrikrudstrom
Copy link
Member Author

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.
If we make one test suite pr component (in Render folder or together with inspection tests). the same file editing shouldnt be that much of an issue.

Render/QtQuick/Rectangle.js
Render/QtQuick/qml/RectangleSomething.qml
Render/QtQuick/qml/RectangleSomething.png

The advantages is see are:

  • more flexible handling of test cases (like Async vs Simple now, more Complex later)
  • we can specify which .qml file is a test if we need to define a sub qml file that cannot be rendered independently.
  • can add (temp) debug code to individual tests

@ChALkeR ChALkeR added the discuss label Mar 7, 2016
@henrikrudstrom
Copy link
Member Author

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.

Rectangle {
    width: 20
    height: 20
    Text {
        id: text
        color: "black"
        font.family: "Arial"
        font.pointSize: 14
        text: "hi"
        anchors.fill: parent
    }
    // Is called from test code or screenshot generator
    function test(){
        // if `testScreenshot` is called from testing code it compares
        // screenshots with the given suffix, if its called from the screenshot utility
        // it generates a screenshot with a suffix. 
        testScreenshot("", tl)
    }
    //subsequent tests are chained
    function tl(){
      text.verticalAlignment = Text.AlignTop
      text.horizontalAlignment = Text.AlignLeft
      testScreenshot("tl", cc)
      // yield value and execution back to testing code.
      // useful for testing timing related issues
      testYield("something")
    }
    function cc(){
      text.verticalAlignment = Text.AlignVCenter
      text.horizontalAlignment = Text.AlignHCenter
      testScreenshot("cc", br)
    }
    function br(){
      text.verticalAlignment = Text.AlignBottom
      text.horizontalAlignment = Text.AlignRight
      // Test done must be called at the end of the last test
      testScreenshot("br", testDone)
    }
}

On the test side it looks like this:

    var qml = loadQmlFile(file, div);
    // these functions is registred to the root object
    // its definition is shared with the screenshot utility
    qml.testScreenshot = function(suffix, callback) {

    };
    qml.testYield = function(obj){
      // Handle yield callbacks
      expect(....
      callback()
    }
    qml.testDone = done;

    // Run test function defined in qml
    qml.test()

i have it halfway working, any thoughts @ChALkeR

@ChALkeR
Copy link
Member

ChALkeR commented Jan 13, 2017

I don't think we should make render tests more complex.
Instead, we should make a small testcase that could be used with Qt to check that all those tests pass on Qt, like qmltestrunner -input tests/Auto/ for Auto tests.

@pavelvasev
Copy link
Contributor

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].

@ChALkeR
Copy link
Member

ChALkeR commented Jan 13, 2017

@pavelvasev Are you speaking about render tests or some other tests?

Render tests are basically a pair of files for each test — a *.qml and a *.png file in the same folder: https://github.com/qmlweb/qmlweb/tree/master/tests/Render/Simple

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.

@ChALkeR
Copy link
Member

ChALkeR commented Jan 15, 2017

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.
Long-term, we should migrate all tests to either Qt-compatible QtTest-based tests or the render tests that could be also run with 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.

@ChALkeR ChALkeR closed this Jan 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants