Enable screenshot functionality (closes #104) #130

AlexanderMoskovkin · 2015-10-23T14:39:29Z

We have client tests for this and server tests for reporting with screenshots now. Also I've created the functional tests locally, but we will commit them when we implement the #52 issue, because we need something like functional-tests-harness

/cc @inikulin @helen-dikareva

testcafe-build-bot · 2015-10-23T14:48:33Z

❌ Tests for the commit ca708d3 have failed. See details.

testcafe-build-bot · 2015-10-23T14:59:14Z

✅ Tests for the commit ca708d3 have passed. See details.

helen-dikareva · 2015-10-26T08:47:33Z

What about steps with empty test names? For all of them will be saved pageLoad.png

inikulin · 2015-10-26T12:01:20Z

src/client/test-run/index.js.mustache

@@ -41,6 +41,7 @@
        testCafeCore.SETTINGS.set({
            CURRENT_TEST_STEP_NAME:   nextStep ? stepNames[nextStep - 1] : 'Test initialization',
            BROWSER_STATUS_URL:       '{{{browserStatusUrl}}}',
+            ENABLE_SCREENSHOTS:       {{{enableScreenshots}}},


I don't like enableScreenshots, maybe takeScreenshots? \cc @VasilyStrelyaev

inikulin · 2015-10-26T12:20:13Z

BTW, I don't see code which attaches screenshot path on errors. And we need to add both types of screenshots in report-design-viewer (we need to figure out coloring for them as well).

inikulin · 2015-10-26T12:20:16Z

\r-

AlexanderMoskovkin · 2015-10-26T12:27:22Z

BTW, I don't see code which attaches screenshot path on errors.

If we implement this behavior we need not any sprecific code which attaches screenshot path on errors. We set the screenshotCreated (hasScreenshots) flag when any screenshot created (on the screenshot action or on an error)

inikulin · 2015-10-26T12:32:35Z

@AlexanderMoskovkin We've discussed what if there were an error, then screenshot should be attached to the error text: #104 (comment)

So the report should look like this:

Running tests in: Chrome, Firefox

  fixture1 (./fixture1.js)
    ✓ fixture1test1 (unstable) (screenshots: D:\screenshots\1445437598847)
    ✖ fixture1test2 (screenshots: D:\screenshots\144543754328847)

        1) Chrome
           Assertion failed at step "Step":

               eq(["12345678901"], ["00000000000"])

           Arrays differ at index 0:

           Expected: [0]: "12345678901"
           Actual:   [0]: "00000000000"
                           ^

           Screenshot: D:\screenshots\144543754328847\errors\1.png


        2) Chrome
           Assertion failed at step "Step":

               notEq("test", "test")

           Expected: not "test"
           Actual:   "test"

           Screenshot: D:\screenshots\144543754328847\errors\2.png

        3) Firefox
           Assertion failed at step "Step":

               ok(false)

           Expected: not null, not undefined, not false, not NaN and not
           ''
           Actual:   false

           Screenshot: D:\screenshots\144543754328847\errors\3.png

    ✓ fixture1test3

  fixture2 (./fixture2.js)
    ✓ fixture2test1 (screenshots: D:\screenshots\4345437598847)
    ✓ fixture2test2 (screenshots: D:\screenshots\6545437598847)


  2/6 failed (15m 25s)

Sorry, seems like I was not quite clear in our discussion.

AlexanderMoskovkin · 2015-10-26T12:34:13Z

ok

AlexanderMoskovkin · 2015-10-27T14:24:00Z

What about steps with empty test names? For all of them will be saved pageLoad.png

I suppose, we should save the image as Step <num>.png in this case, for example Step 3.png.
Do step names should start from Step 0 or Step 1?
/cc @inikulin @VasilyStrelyaev

inikulin · 2015-10-27T14:48:38Z

@AlexanderMoskovkin AFAIK you can't create step without a name in current API. Unnamed steps are subject of the new API and we will have a lot of other things to redo because of this. So, I think we shouldn't bother right now.

AlexanderMoskovkin · 2015-10-27T15:00:19Z

I've checked it works with empty step names now. For example if an assertion failed in this step we see in the report Assertion failed at step "". But I think we can skip it now because we will implement the new API

AlexanderMoskovkin · 2015-10-28T08:47:29Z

/cc @inikulin @VasilyStrelyaev
I've tried some colors for screenshot paths in reports, what do you think? I suppose, we should use a neutral color because it's just an info and we should not use such colors as red(error), yellow(warning), green(success). Screens from Windows:

VasilyStrelyaev · 2015-10-28T10:00:41Z

yellow is not too much of warning
it's rather highlighting - we use it to mark inline code and so on
I'd go for yellow or no accent (the third screenshot)

AlexanderMoskovkin · 2015-10-28T10:26:55Z

Yellow:

testcafe-build-bot · 2015-10-29T16:32:05Z

✅ Tests for the commit 9d3f6eb have passed. See details.

AlexanderMoskovkin · 2015-10-30T08:35:36Z

FPR
/cc @inikulin @helen-dikareva
@VasilyStrelyaev please check the comment here

Current reporting:

VasilyStrelyaev · 2015-10-30T09:24:39Z

src/runner/test-run/index.js

+                   `${this.screenshotPath}\\${isFailedStep ? 'errors\\' : ''}${stepName.replace(/\s/g, '_') ||
+                                                                               'Page_Load'}.png`;
+
+    //NOTE: we should not fail the test if we can't make a screenshot for some reason


The test should not fail if we can't make a screenshot for some reason

VasilyStrelyaev · 2015-10-30T09:24:50Z

/r-

testcafe-build-bot · 2015-10-30T14:03:16Z

✅ Tests for the commit c804a48 have passed. See details.

VasilyStrelyaev · 2015-10-30T14:12:52Z

lgtm

AlexanderMoskovkin · 2015-10-31T18:48:46Z

FPR

VasilyStrelyaev · 2015-11-13T12:17:10Z

src/client/core/transport.js

@@ -13,26 +16,23 @@ export var batchUpdate                     = transport.batchUpdate.bind(transpor
 export var queuedAsyncServiceMsg           = transport.queuedAsyncServiceMsg.bind(transport);

 export function fail (err, callback) {
+    // NOTE: we should not stop the test run if an error occured during page unloading because
+    // we destroy the session in this case and we can't get the next page in the browser.
+    // We should set the deferred error to the task to fail the test after the page reloading.


// NOTE: we should not stop the test run if an error occured during page unloading because // we would destroy the session in this case and wouldn't be able to get the next page in the browser. // We should set the deferred error to the task to have the test failed after the page reloading.

VasilyStrelyaev · 2015-11-13T12:32:20Z

/r-

testcafe-build-bot · 2015-11-16T08:52:28Z

✅ Tests for the commit b0108e1 have passed. See details.

VasilyStrelyaev · 2015-11-16T09:05:50Z

src/client/core/transport.js

+export function fatalError (err, callback) {
+    // NOTE: we should not stop the test run if an error occured during page unloading because we
+    // would destroy the session in this case and wouldn't be able to get the next page in the browser.
+    // We should set the deferred error to the task to have the test failed after the page reloading.


sht... sorry, my mistake - have the test fail

// We should set the deferred error to the task to have the test fail after the page reloading.

VasilyStrelyaev · 2015-11-16T09:11:44Z

/r-

testcafe-build-bot · 2015-11-16T09:29:09Z

✅ Tests for the commit bee81e7 have passed. See details.

VasilyStrelyaev · 2015-11-16T09:45:45Z

lgtm

AlexanderMoskovkin · 2015-11-16T09:53:02Z

FPR

inikulin · 2015-11-16T13:04:31Z

lgtm

inikulin · 2015-11-16T14:28:27Z

ping @helen-dikareva

helen-dikareva · 2015-11-16T14:32:32Z

src/client/runner/runner.js

+            this.savedDocumentTitle = document.title;
+            document.title          = assignedTitle;
+        }
+    }, 50);


Let's move it to const

helen-dikareva · 2015-11-16T15:17:02Z

\r-

testcafe-build-bot · 2015-11-17T09:46:47Z

✅ Tests for the commit 1768a35 have passed. See details.

AlexanderMoskovkin · 2015-11-17T09:46:53Z

FPR

inikulin · 2015-11-17T09:47:48Z

lgtm

helen-dikareva · 2015-11-17T10:20:34Z

lgtm

…nctionality Enable screenshot functionality (closes #104)

AlexanderMoskovkin · 2015-11-17T10:22:00Z

thanks

inikulin reviewed Oct 26, 2015
View reviewed changes

AlexanderMoskovkin force-pushed the enable-screenshots-functionality branch from ca708d3 to 9d3f6eb Compare October 29, 2015 16:10

VasilyStrelyaev reviewed Oct 30, 2015
View reviewed changes

AlexanderMoskovkin force-pushed the enable-screenshots-functionality branch from 9d3f6eb to c804a48 Compare October 30, 2015 13:45

VasilyStrelyaev reviewed Nov 13, 2015
View reviewed changes

AlexanderMoskovkin force-pushed the enable-screenshots-functionality branch 2 times, most recently from 790d080 to b0108e1 Compare November 16, 2015 08:39

VasilyStrelyaev reviewed Nov 16, 2015
View reviewed changes

AlexanderMoskovkin force-pushed the enable-screenshots-functionality branch from b0108e1 to bee81e7 Compare November 16, 2015 09:13

AlexanderMoskovkin mentioned this pull request Nov 16, 2015

Uncaught error in JSON.parse which brakes test task #160

Closed

helen-dikareva reviewed Nov 16, 2015
View reviewed changes

Enable screenshot functionality (closes DevExpress#104)

1768a35

AlexanderMoskovkin force-pushed the enable-screenshots-functionality branch from bee81e7 to 1768a35 Compare November 17, 2015 09:32

helen-dikareva added a commit that referenced this pull request Nov 17, 2015

Merge pull request #130 from AlexanderMoskovkin/enable-screenshots-fu…

92f91cf

…nctionality Enable screenshot functionality (closes #104)

helen-dikareva merged commit 92f91cf into DevExpress:master Nov 17, 2015

AlexanderMoskovkin deleted the enable-screenshots-functionality branch November 17, 2015 11:25

Enable screenshot functionality (closes #104) #130

Enable screenshot functionality (closes #104) #130

Conversation

AlexanderMoskovkin commented Oct 23, 2015

testcafe-build-bot commented Oct 23, 2015

testcafe-build-bot commented Oct 23, 2015

helen-dikareva commented Oct 26, 2015

inikulin Oct 26, 2015

Choose a reason for hiding this comment

VasilyStrelyaev Oct 26, 2015

Choose a reason for hiding this comment

inikulin commented Oct 26, 2015

inikulin commented Oct 26, 2015

AlexanderMoskovkin commented Oct 26, 2015

inikulin commented Oct 26, 2015

AlexanderMoskovkin commented Oct 26, 2015

AlexanderMoskovkin commented Oct 27, 2015

inikulin commented Oct 27, 2015

AlexanderMoskovkin commented Oct 27, 2015

AlexanderMoskovkin commented Oct 28, 2015

VasilyStrelyaev commented Oct 28, 2015

AlexanderMoskovkin commented Oct 28, 2015

testcafe-build-bot commented Oct 29, 2015

AlexanderMoskovkin commented Oct 30, 2015

VasilyStrelyaev Oct 30, 2015

Choose a reason for hiding this comment

VasilyStrelyaev commented Oct 30, 2015

testcafe-build-bot commented Oct 30, 2015

VasilyStrelyaev commented Oct 30, 2015

AlexanderMoskovkin commented Oct 31, 2015

VasilyStrelyaev Nov 13, 2015

Choose a reason for hiding this comment

VasilyStrelyaev commented Nov 13, 2015

testcafe-build-bot commented Nov 16, 2015

VasilyStrelyaev Nov 16, 2015

Choose a reason for hiding this comment

VasilyStrelyaev commented Nov 16, 2015

testcafe-build-bot commented Nov 16, 2015

VasilyStrelyaev commented Nov 16, 2015

AlexanderMoskovkin commented Nov 16, 2015

inikulin commented Nov 16, 2015

inikulin commented Nov 16, 2015

helen-dikareva Nov 16, 2015

Choose a reason for hiding this comment

inikulin Nov 16, 2015

Choose a reason for hiding this comment

helen-dikareva commented Nov 16, 2015

testcafe-build-bot commented Nov 17, 2015

AlexanderMoskovkin commented Nov 17, 2015

inikulin commented Nov 17, 2015

helen-dikareva commented Nov 17, 2015

AlexanderMoskovkin commented Nov 17, 2015