Skip to content
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.

Sign up for GitHub

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

Jump to bottom

try to fix two UI tests #15755

Merged
merged 19 commits into from Apr 19, 2020
Merged

try to fix two UI tests #15755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
580e7a4
try to fix two UI tests
diosmosis Mar 31, 2020
4ed6ad4
update screenshots
diosmosis Mar 31, 2020
246de42
try to fix oneclickupdate
diosmosis Mar 31, 2020
8d94010
try to fix build
diosmosis Apr 1, 2020
ede1376
remove debugging code
diosmosis Apr 2, 2020
121010f
Merge branch '4.x-dev' into fix-build
sgiehl Apr 6, 2020
f9fe300
updates ui files
sgiehl Apr 6, 2020
9881cdc
remove debugging code
diosmosis Apr 6, 2020
3e25e98
Revert "try to fix oneclickupdate"
diosmosis Apr 8, 2020
916afa5
revert oneclickupdate change
diosmosis Apr 8, 2020
cb9ceb9
Merge branch '4.x-dev' into fix-build
diosmosis Apr 8, 2020
003e77f
Use 4.x-dev TagManager submodule.
diosmosis Apr 8, 2020
b27f4e7
Merge branch '4.x-dev' into fix-build
diosmosis Apr 15, 2020
14d12b2
Merge branch '4.x-dev' into fix-build
diosmosis Apr 16, 2020
3c65b07
Use temp token auth in one click update to make sure second request a…
diosmosis Apr 16, 2020
e4b3ff2
Merge branch '4.x-dev' into fix-build
diosmosis Apr 19, 2020
c9a8d4f
try to fix random failure
diosmosis Apr 19, 2020
2e427eb
add expected screenshot
diosmosis Apr 19, 2020
d677b5a
try update again
diosmosis Apr 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions plugins/CoreUpdater/Updater.php
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@
use Piwik\Filesystem;
use Piwik\Http;
use Piwik\Option;
use Piwik\Piwik;
use Piwik\Plugin\Manager as PluginManager;
use Piwik\Plugin\ReleaseChannels;
use Piwik\Plugins\CorePluginsAdmin\PluginInstaller;
Expand Down Expand Up @@ -124,8 +125,10 @@ public function updatePiwik($https = true)
throw new UpdaterException($e, $messages);
}

$tempTokenAuth = Piwik::requestTemporarySystemAuthToken('OneClickUpdate', 1);
$partTwoUrl = Url::getCurrentUrlWithoutQueryString() . Url::getCurrentQueryStringWithParametersModified([
'action' => 'oneClickUpdatePartTwo',
'token_auth' => $tempTokenAuth,
]);

$response = Http::sendHttpRequest($partTwoUrl, 300);
Expand Down
4 changes: 4 additions & 0 deletions plugins/UsersManager/tests/UI/UserSettings_spec.js
View file
Open in desktop
Expand Up @@ -23,6 +23,10 @@ describe("UserSettings", function () {

it('should show user security page', async function () {
await page.goto(userSecurityUrl);
await page.waitFor('.listAuthTokens', { visible: true });
await page.evaluate(() => { // give table headers constant width so the screenshot stays the same
$('table.listAuthTokens th').css('width', '25%');
});
await page.waitFor(100);
expect(await page.screenshotSelector('.admin')).to.matchImage('load_security');
});
Expand Down
4 changes: 2 additions & 2 deletions plugins/UsersManager/tests/UI/expected-screenshots/UserSettings_load_security.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
20 changes: 20 additions & 0 deletions tests/PHPUnit/Fixtures/UITestFixture.php
View file
Open in desktop
Expand Up @@ -217,13 +217,33 @@ private function addOverlayVisits()
"95.81.66.139",
);

$userAgents = [
'Mozilla/5.0 (Linux; Android 4.4.2; Nexus 4 Build/KOT49H) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.136 Mobile Safari/537.36',
'Mozilla/5.0 (Linux; U; Android 2.3.7; fr-fr; HTC Desire Build/GRI40; MildWild CM-8.0 JG Stable) AppleWebKit/533.1 (KHTML, like Gecko) Version/4.0 Mobile Safari/533.1',
'Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.76 Safari/537.36',
'Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.0; Trident/4.0; GTB6.3; Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1) ; SLCC1; .NET CLR 2.0.50727; Media Center PC 5.0; .NET CLR 3.5.30729; .NET CLR 3.0.30729; OfficeLiveConnector.1.4; OfficeLivePatch.1.3)',
'Mozilla/5.0 (Windows NT 6.1; Trident/7.0; MDDSJS; rv:11.0) like Gecko',
'Mozilla/5.0 (Linux; Android 4.1.1; SGPT13 Build/TJDS0170) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.114 Safari/537.36',
'Mozilla/5.0 (Linux; U; Android 4.3; zh-cn; SM-N9006 Build/JSS15J) AppleWebKit/537.36 (KHTML, like Gecko)Version/4.0 MQQBrowser/5.0 Mobile Safari/537.36',
'Mozilla/5.0 (X11; U; Linux i686; ru; rv:1.9.0.14) Gecko/2009090216 Ubuntu/9.04 (jaunty) Firefox/3.0.14',
'Mozilla/5.0 (iPhone; CPU iPhone OS 10_3_1 like Mac OS X) AppleWebKit/603.1.30 (KHTML, like Gecko) Version/10.0 Mobile/14E304 Safari/602.1',
'Mozilla/5.0 (Linux; U; Android 4.4.2; en-us; SCH-I535 Build/KOT49H) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 Mobile Safari/534.30',
'Mozilla/5.0 (Linux; Android 7.0; SM-G930V Build/NRD90M) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.125 Mobile Safari/537.36',
'Mozilla/5.0 (Linux; Android 7.0; SM-A310F Build/NRD90M) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.91 Mobile Safari/537.36 OPR/42.7.2246.114996',
'Opera/9.80 (J2ME/MIDP; Opera Mini/5.1.21214/28.2725; U; ru) Presto/2.8.119 Version/11.10',
'Mozilla/5.0 (iPhone; CPU iPhone OS 10_3_2 like Mac OS X) AppleWebKit/603.2.4 (KHTML, like Gecko) FxiOS/7.5b3349 Mobile/14F89 Safari/603.2.4',
'Mozilla/5.0 (Android 7.0; Mobile; rv:54.0) Gecko/54.0 Firefox/54.0',
'Mozilla/5.0 (Linux; Android 6.0.1; SM-G920V Build/MMB29K) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.98 Mobile Safari/537.36',
];

$date = Date::factory('yesterday');
$t = self::getTracker($idSite = 3, $dateTime = $date->getDatetime(), $defaultInit = true);
$t->enableBulkTracking();

foreach ($visitProfiles as $visitCount => $visit) {
$t->setNewVisitorId();
$t->setIp($ips[$visitCount]);
$t->setUserAgent($userAgents[$visitCount]);

foreach ($visit as $idx => $action) {
$t->setForceVisitDateTime($date->addHour($visitCount)->addHour(0.01 * $idx)->getDatetime());
Expand Down
4 changes: 2 additions & 2 deletions tests/UI/expected-screenshots/OneClickUpdate_login.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 2 additions & 2 deletions tests/UI/expected-screenshots/UIIntegrationTest_visitors_realtime_visits.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 6 additions & 0 deletions tests/UI/specs/OneClickUpdate_spec.js
View file
Open in desktop
Expand Up @@ -75,6 +75,12 @@ describe("OneClickUpdate", function () {
}
}

await page.waitFor('#login_form_login', { visible: true });

await page.type('#login_form_login', 'superUserLogin');
await page.type('#login_form_password', 'superUserPass');
await page.click('#login_form_submit');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So automatic login after update won't work anymore? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What automatic login are referring to? From my understanding the session is just sent again after the update, but I think it's when updating to 4.x due the app specific token auth change it requires a new login. Otherwise anyone would get logged in even if they aren't authorized to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I meant. Guess it's not worth to have a closer look if it's possible to not require a new login...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsteur might know if the session should still work

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's ok that the session gets terminated, the changes would be good to merge

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I think it's when updating to 4.x due the app specific token auth change it requires a new login. Otherwise anyone would get logged in even if they aren't authorized to?

Not sure I fully understand the problem @sgiehl @diosmosis

Is this something that happens for the actual user or is more of a problem only for tests? Generally, when updating, the current token_auth should be automatically be migrated to an app specific token and still work. I suppose the token_auth is not really used for the session though.

What does change is indeed the session cookie maybe as we're removing login and token hash or so from the cookie AFAIK. Not sure if this causes the issue?

If user gets logged out would this cause an issue for https://github.com/matomo-org/matomo/pull/15770/files#diff-adf51e994c74a650d8afec954288d4c3R178 where we require user to be logged in? Not sure if user can be logged out in between (either by this session change or because the cookie expire time which might expire after 30 minutes just in between steps).

Looking at the original PR for app specific tokens, I can only imagine that maybe this session value is missing and causing an issue (random guess) https://github.com/matomo-org/matomo/pull/15410/files#diff-6c322e8c4c05670597358309f152c51aR94

Generally I would say it's fine though if a user gets logged out during the update I would say. As long as it's possible to complete the update fully.

Copy link
Member Author

@diosmosis diosmosis Apr 14, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If user gets logged out would this cause an issue for https://github.com/matomo-org/matomo/pull/15770/files#diff-adf51e994c74a650d8afec954288d4c3R178 where we require user to be logged in?

It might be... though we only make the request internally in Matomo, so maybe we can create a temporary token auth for that request to make sure it works?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could work @diosmosis

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsteur made the change after merging in changes from 3.x, can review it again

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


await page.waitFor('.site-without-data', { visible: true });
await page.waitForNetworkIdle();

Expand Down
6 changes: 4 additions & 2 deletions tests/UI/specs/UIIntegration_spec.js
View file
Open in desktop
Expand Up @@ -12,8 +12,10 @@ describe("UIIntegrationTest", function () { // TODO: Rename to Piwik?

var generalParams = 'idSite=1&period=year&date=2012-08-09',
idSite2Params = 'idSite=2&period=year&date=2012-08-09',
idSite3Params = 'idSite=3&period=year&date=2012-08-09',
evolutionParams = 'idSite=1&period=day&date=2012-01-31&evolution_day_last_n=30',
urlBase = 'module=CoreHome&action=index&' + generalParams,
urlBaseGeneric = 'module=CoreHome&action=index&',
urlBase = urlBaseGeneric + generalParams,
widgetizeParams = "module=Widgetize&action=iframe",
segment = encodeURIComponent("browserCode==FF") // from OmniFixture
;
Expand Down Expand Up @@ -235,7 +237,7 @@ describe("UIIntegrationTest", function () { // TODO: Rename to Piwik?
});

it('should load the visitors > real-time visits page correctly', async function () {
await page.goto("?" + urlBase + "#?" + idSite2Params + "&category=General_Visitors&subcategory=General_RealTime");
await page.goto("?" + urlBaseGeneric + idSite3Params + "#?" + idSite3Params + "&category=General_Visitors&subcategory=General_RealTime");
await page.mouse.move(-10, -10);

pageWrap = await page.$('.pageWrap');
Expand Down