fix 'Page without slash after location's host reloads if we set only hash' (close #1426) #1514

Farfurix · 2018-02-27T13:30:37Z

Reference: DevExpress/testcafe#2005

Example test is passed (DevExpress/testcafe#2005)

miherlosev · 2018-02-27T13:39:31Z

src/client/index.js

            return;

-        this.win.location = urlUtils.getProxyUrl(url);
+        const proxyUrl = urlUtils.getProxyUrl(ensureHostEndedTrailingSlash(url));


1)Rewrite in two lines of code:

url = ensureHostEndedTrailingSlash(url); const proxyUrl = urlUtils.getProxyUrl(url);

2)Host, port and protocol is named as 'origin'. See https://developer.mozilla.org/ru/docs/Web/API/Location.
So, let's rename function to ensureOriginTrailingSlash(url).
Add this comment in function body

If you request an url containing only port, host and protocol then browser adds the trailing slash itself.

Done. May be add window.location.href in comment?

// NOTE: If you request an url containing only port, host and protocol // then browser adds the trailing slash to window.location.href.

testcafe-build-bot · 2018-02-27T13:51:10Z

❌ Tests for the commit 80f4359 have failed. See details:

testcafe-build-bot · 2018-02-28T06:07:55Z

❌ Tests for the commit e73c9b1 have failed. See details:

testcafe-build-bot · 2018-02-28T08:49:42Z

❌ Tests for the commit 7da06bd have failed. See details:

testcafe-build-bot · 2018-02-28T14:09:27Z

✅ Tests for the commit 09f8e24 have passed. See details:

testcafe-build-bot · 2018-03-01T06:52:17Z

✅ Tests for the commit f324dd6 have passed. See details:

testcafe-build-bot · 2018-03-01T11:26:24Z

❌ Tests for the commit e1696c9 have failed. See details:

Farfurix · 2018-03-01T11:37:52Z

@testcafe-build-bot \retest

testcafe-build-bot · 2018-03-01T12:04:20Z

✅ Tests for the commit e1696c9 have passed. See details:

Farfurix · 2018-03-05T09:23:27Z

@testcafe-build-bot \retest

testcafe-build-bot · 2018-03-05T10:31:22Z

❌ Tests for the commit 5b17b49 have failed. See details:

testcafe-build-bot · 2018-03-05T10:32:52Z

✅ Tests for the commit 5b17b49 have passed. See details:

miherlosev · 2018-03-05T11:12:24Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+    'https://127.0.0.1',
+    'http://127.0.0.1:8080',
+    'https://127.0.0.1:8080'
+];


Let's reduce testcases and combine them to the single contant:

var testcases = [ { url: 'http://example.com' /*protocol + host*/ shoudAddTrailingSlash: true }, { url: https://localhost:8080 /*protocol + host + port*/, shoudAddTrailingSlash: true }, { url: about:blank /*special page*/, shoudAddTrailingSlash: false }, { url: http://example.com/page.html, /*url with path*/ shoudAddTrailingSlash: false }, linux file protocol url, window file protocol url ];

miherlosev · 2018-03-05T11:15:02Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+
+            var locationWrapper = new LocationWrapper(windowMock);
+
+            strictEqual(locationWrapper.href, url + (trailingSlashIsNeeded ? '/' : ''));


It's a wrong test because we add a trailing slash on href property setter.

Done, new test: 'should add the trailing slash to location.href using href.set(), assign() and replace() functions if url consist of origin (GH-1426)'

miherlosev · 2018-03-05T11:20:27Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+        };
+    };
+
+    var windowMock = getWindowMock();


Bad test algorithm.
window does not contain the navigateTo method.
Rewrite as:

var storedWindow = hammerhead.win; hammerhead.win = windowMock; test action with hammerhead.navigateTo() restore hammerhead.win = storedWindow;

miherlosev · 2018-03-05T11:21:31Z

test/client/fixtures/utils/url-test.js

@@ -105,6 +105,41 @@ test('isRelativeUrl', function () {
    ok(sharedUrlUtils.isRelativeUrl('C:\\index.htm'));
 });

+test('ensureOriginTrailingSlash', function () {
+    const SHOULD_ADD_TRAILING_SLASH = [


Use testcases from the comment above.

miherlosev · 2018-03-05T11:37:08Z

test/server/proxy-test.js

@@ -296,6 +296,13 @@ describe('Proxy', () => {
                .end();
        });

+        app.get('/location-header-with-trailing-slash/:url', (req, res) => {


Don't dublicate routes without necessary.
See

testcafe-hammerhead/test/server/proxy-test.js

Line 292 in 8b63395

app.get('/redirect/:url', (req, res) => {

miherlosev · 2018-03-05T11:38:26Z

test/server/proxy-test.js

+            }
+
+            return Promise.all([
+                testRedirectRequest('http://example.com', true),


Reduce testcases count according the comment above.

…ginTrailingSlash(); rewrite urlUtils.getProxyUrl(ensureOriginTrailingSlash(url))

…export TRAILING_SLASH_RE needed in this test, fix client location test

… header test, add location href test (wrapper)

testcafe-build-bot · 2018-03-07T09:22:07Z

❌ Tests for the commit f28b840 have failed. See details:

Farfurix · 2018-03-07T09:44:22Z

@testcafe-build-bot \retest

testcafe-build-bot · 2018-03-07T10:10:44Z

✅ Tests for the commit f28b840 have passed. See details:

miherlosev · 2018-03-07T12:25:53Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

@@ -157,6 +192,83 @@ test('special pages (GH-339)', function () {
    destLocation.forceLocation(storedForcedLocation);
 });

+test('should add the trailing slash to location.href using href.set(), assign() and replace() functions if url consist of origin (GH-1426)', function () {


should ensure a trailing slash on page navigation using href setter, assign and replace methods (GH-1426)

miherlosev · 2018-03-07T12:26:22Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+        urlUtils.getProxyUrl(url);
+    }
+
+    function proxyUrlTrailingSlash (urlCase) {


function getExpectedProxyUrl(testCase)

miherlosev · 2018-03-07T12:27:47Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+            replace:  setHref,
+            assign:   setHref,
+            toString: function () {
+                return proxyUrl(this.location.href);


use inplace declaration urlUtils.getProxyUrl(url)

miherlosev · 2018-03-07T12:28:13Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+    var windowMock = {
+        location: {
+            href:     '',
+            replace:  setHref,


use inplace declaration this.href = url; in two places

miherlosev · 2018-03-07T12:30:37Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

@@ -11,6 +11,41 @@ var nativeMethods = hammerhead.nativeMethods;
 var browserUtils  = hammerhead.utils.browser;
 var domUtils      = hammerhead.utils.dom;

+var URL_TEST_CASES = [


var ENSURE_URL_TRAILING_SLASH_TEST_CASES = ...

miherlosev · 2018-03-07T12:31:36Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+    checkTrailingSlash(URL_TEST_CASES);
+});
+
+test('should add the trailing slash to location when navigateTo() is called (GH-1426)', function () {


should ensure a trailing slash on page navigation using hammerhead.navigateTo method (GH-1426)

miherlosev · 2018-03-07T12:32:19Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+test('should add the trailing slash to location when navigateTo() is called (GH-1426)', function () {
+    var storedWindow = hammerhead.win;
+
+    function proxyUrlTrailingSlash (urlCase) {


Rename according the comment above.

Also need to investigate a necessary of ensureTrailingSlash function. As I understand, we can remove it in this PR.

testcafe-build-bot · 2018-03-07T15:19:12Z

✅ Tests for the commit 9b4fc9f have passed. See details:

miherlosev · 2018-03-13T08:21:57Z

src/utils/url.js

+
+export function ensureOriginTrailingSlash (url) {
+    // NOTE: If you request an url containing only port, host and protocol
+    // then browser adds the trailing slash to window.location.href.


Change
...then browser adds the trailing slash to window.location.href.
to
...then browser adds the trailing slash itself

miherlosev · 2018-03-13T08:22:37Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+
+    var locationWrapper = new LocationWrapper(windowMock);
+
+    function checkTrailingSlash (testCases) {


function testAddingTrailingSlash (testCases) {
here and below

miherlosev · 2018-03-13T08:24:04Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+        };
+    };
+
+    var windowMock = getWindowMock();


getWindowMock function is necessary.
Rewrite as

var windowMock = { location: ''; };

miherlosev · 2018-03-13T08:26:03Z

test/client/fixtures/utils/url-test.js

@@ -105,6 +105,53 @@ test('isRelativeUrl', function () {
    ok(sharedUrlUtils.isRelativeUrl('C:\\index.htm'));
 });

+test('ensureOriginTrailingSlash', function () {


Let's remove this test.

miherlosev · 2018-03-13T08:31:45Z

test/server/proxy-test.js

+                    });
+            };
+
+            return Promise.all(proxyUrls.map(testPageRequest));


Rename testPageRequest to testLocationHeaderValue.

Combine proxyUrls with testPageRequest.

Returned Promise should looks like return Promise.all(ENSURE_URL_TRAILING_SLASH_TEST_CASES.map(testLocationHeaderValue))

testcafe-build-bot · 2018-03-13T11:48:39Z

✅ Tests for the commit 2b45e72 have passed. See details:

miherlosev · 2018-03-13T12:31:01Z

@LavrovArtem review too

…hash' (close DevExpress#1426) (DevExpress#1514) * i1426 * i1426 * Requested changes: rename ensureHostEndedTrailingSlash() to ensureOriginTrailingSlash(); rewrite urlUtils.getProxyUrl(ensureOriginTrailingSlash(url)) * Fix server test (Should procees "x-frame-options" header (DevExpressGH-1017)), export TRAILING_SLASH_RE needed in this test, fix client location test * Fix navigateTo(): leading slashes case * Fix ensureOriginTrailingSlash(), code cleanup * Add ensureOriginTrailingSlash test * Fix location header, fix ensureOriginTrailingSlash test, add location header test, add location href test (wrapper) * Add tests * Requested changes * Requested changes * Requested changes

Farfurix requested a review from miherlosev February 27, 2018 13:30

miherlosev requested changes Feb 27, 2018

View reviewed changes

Farfurix changed the title ~~[WIP] fix 'Page without slash after location's host reloads if we set only hash' (close #1426)~~ fix 'Page without slash after location's host reloads if we set only hash' (close #1426) Mar 5, 2018

miherlosev requested changes Mar 5, 2018

View reviewed changes

Farfurix added 10 commits March 7, 2018 11:56

i1426

a56994c

i1426

3009b4f

Requested changes: rename ensureHostEndedTrailingSlash() to ensureOri…

20c1b2e

…ginTrailingSlash(); rewrite urlUtils.getProxyUrl(ensureOriginTrailingSlash(url))

Fix server test (Should procees "x-frame-options" header (GH-1017)), …

e618898

…export TRAILING_SLASH_RE needed in this test, fix client location test

Fix navigateTo(): leading slashes case

101d784

Fix ensureOriginTrailingSlash(), code cleanup

8b56ed4

Add ensureOriginTrailingSlash test

bf2cf94

Fix location header, fix ensureOriginTrailingSlash test, add location…

f06ac20

… header test, add location href test (wrapper)

Add tests

9259a1f

Requested changes

c704294

miherlosev requested changes Mar 7, 2018

View reviewed changes

Requested changes

9b4fc9f

miherlosev requested changes Mar 13, 2018

View reviewed changes

Requested changes

2b45e72

miherlosev approved these changes Mar 13, 2018

View reviewed changes

LavrovArtem approved these changes Mar 13, 2018

View reviewed changes

miherlosev merged commit 7639504 into DevExpress:master Mar 13, 2018

Farfurix deleted the i1426 branch April 10, 2018 05:35


		var locationWrapper = new LocationWrapper(windowMock);

		strictEqual(locationWrapper.href, url + (trailingSlashIsNeeded ? '/' : ''));


		var locationWrapper = new LocationWrapper(windowMock);

		function checkTrailingSlash (testCases) {

fix 'Page without slash after location's host reloads if we set only hash' (close #1426) #1514

fix 'Page without slash after location's host reloads if we set only hash' (close #1426) #1514

Conversation

Farfurix commented Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testcafe-build-bot commented Feb 27, 2018

testcafe-build-bot commented Feb 28, 2018

testcafe-build-bot commented Feb 28, 2018

testcafe-build-bot commented Feb 28, 2018

testcafe-build-bot commented Mar 1, 2018

testcafe-build-bot commented Mar 1, 2018

Farfurix commented Mar 1, 2018

testcafe-build-bot commented Mar 1, 2018

Farfurix commented Mar 5, 2018

testcafe-build-bot commented Mar 5, 2018

testcafe-build-bot commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testcafe-build-bot commented Mar 7, 2018

Farfurix commented Mar 7, 2018

testcafe-build-bot commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testcafe-build-bot commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testcafe-build-bot commented Mar 13, 2018

miherlosev commented Mar 13, 2018

Farfurix commented Feb 27, 2018 •

edited

Loading