feat: device-aware logging + screenshots on test failure #55
Conversation
Miki-Session
changed the title
| @@ -15,6 +15,7 @@ import { runScriptAndLog } from './utils/utilities'; | |||
| import { getAdbFullPath } from './utils/binaries'; | |||
| import { closeApp } from './utils/open_app'; | |||
| import { englishStrippedStr } from '../../localizer/englishStrippedStr'; | |||
| import type { TestInfo } from '@playwright/test'; | |||
There was a problem hiding this comment.
usually, we want imports from the node_modules at the top of the file. Maybe check before it gets too bad to add an eslint rules and fix them all (can be a separate PR)
There was a problem hiding this comment.
Added perfectionist and am enforcing sort-import and sort-named-imports now.
| success = true; // Exit the loop if successful | ||
| } else { | ||
| throw new Error(`longPress on conversation list: ${userName} unsuccessful`); | ||
| } | ||
| } catch (error) { | ||
| console.log(`Longpress attempt ${attempt} failed. Retrying...`); | ||
| this.log(`Longpress attempt ${attempt} failed. Retrying...`); |
There was a problem hiding this comment.
have you planned on refactoring all of the others console.* from outside of DeviceWrapper.ts to have as many with a context associated with it?
There was a problem hiding this comment.
I made the devicewrapper logs public and went through all .ts files and added in context-aware logging where it seemed appropriate.
| result.devices[0].setDeviceIdentity('alice1'); | ||
| result.devices[1].setDeviceIdentity('bob1'); | ||
| const seedPhrases = result.prebuilt.users.map(m => m.seedPhrase); | ||
| await linkDevices(result.devices, seedPhrases); | ||
|
|
||
| const alice = result.prebuilt.users[0]; | ||
| const bob = result.prebuilt.users[1]; | ||
| const alice1 = result.devices[0]; | ||
| const bob1 = result.devices[1]; |
There was a problem hiding this comment.
maybe move those just below so it's easier to read and catch mistakes
- result.devices[0].setDeviceIdentity('alice1');
- result.devices[1].setDeviceIdentity('bob1');
const alice1 = result.devices[0];
const bob1 = result.devices[1];
+ alice1.setDeviceIdentity('alice1');
+ bob1.setDeviceIdentity('bob1');... same with the others in this file
| @@ -18,6 +18,7 @@ export const linkedDevice = async ( | |||
| ) => { | |||
| const user = await newUser(device1, userName); | |||
| // Log in with recovery seed on device 2 | |||
| device2.setDeviceIdentity(`${userName.toLowerCase()}2`); | |||
There was a problem hiding this comment.
hopefully we never linked a third device to a user using this linkedDevice function?
If we ever do, alice2 and alice3 will both be named alice2.
Maybe instead make the change to have a 4th argument to linkedDevice, being the deviceIdentity to assign?
There was a problem hiding this comment.
It's impossible to create alice2 and alice3 with this method currently. It first creates a new user (name1) and then logs in with that seed on device 2 (which gets set to name2). If we want to support multi-device-linking then the method needs to be completely rewritten/expanded and then a deviceNumber parameter could make sense.
| export function registerDevicesForTest( | ||
| testInfo: TestInfo, | ||
| devices: DeviceWrapper[], | ||
| platform: SupportedPlatformsType | ||
| ) { | ||
| const testId = `${testInfo.testId}-${testInfo.repeatEachIndex}`; | ||
| deviceRegistry.set(testId, { devices, testInfo, platform }); | ||
| } |
There was a problem hiding this comment.
it's probably safe to check if such an entry exists, and throws if that's the case.
If there is already an entry, it means a unregisterDevicesForTest wasn't called, and that means we are going to include things that have nothing to do there.
There was a problem hiding this comment.
Good catch, added in a check.
| // Position remaining screenshots | ||
| const overlays = []; | ||
|
|
||
| // Top right (device 2) | ||
| if (screenshots[1]) { | ||
| overlays.push({ | ||
| input: screenshots[1], | ||
| left: width + gap, | ||
| top: 0, | ||
| }); | ||
| } | ||
|
|
||
| // Bottom left (device 3) | ||
| if (screenshots[2]) { | ||
| overlays.push({ | ||
| input: screenshots[2], | ||
| left: 0, | ||
| top: height + gap, | ||
| }); | ||
| } | ||
|
|
||
| // Bottom right (device 4) | ||
| if (screenshots[3]) { | ||
| overlays.push({ | ||
| input: screenshots[3], | ||
| left: width + gap, | ||
| top: height + gap, | ||
| }); | ||
| } |
There was a problem hiding this comment.
could be this?
const countPerRow = 2;
screenshots.slice(1).forEach((screenshot, index) => {
overlays.push({
input: screenshot,
left: ((index + 1)% countPerRow )* (width + gap),
top: (Math.floor(index/countPerRow) )(height + gap),
});
})There was a problem hiding this comment.
I feel like this one is the only one that needs to exist.
The others createSideBySideComposite and the single screenshot one are just specific cases.
Can you refactor it to be able to have only one of them?
| const composite = await sharp(left) | ||
| .extend({ | ||
| right: width + 2, // 2px gap | ||
| background: { r: 255, g: 255, b: 255, alpha: 1 }, | ||
| }) | ||
| .composite([ | ||
| { | ||
| input: right, | ||
| left: width + 2, | ||
| top: 0, | ||
| }, | ||
| ]) | ||
| .png() | ||
| .toBuffer(); | ||
|
|
There was a problem hiding this comment.
make this 2 a variable too outside of the function, and reuse it for createSideBySideComposite and createGridComposite
| const metadata = await sharp(left).metadata(); | ||
| const width = metadata.width; |
There was a problem hiding this comment.
search for metadata in all files, but this can be cleaned up with
const {width} = await sharp(left).metadata();
| export function unregisterDevicesForTest(testInfo: TestInfo) { | ||
| const testId = `${testInfo.testId}-${testInfo.repeatEachIndex}`; | ||
| deviceRegistry.delete(testId); | ||
| } |
There was a problem hiding this comment.
Have you tested this with multiple tests running at the same time over multiple workers?
That deviceRegistry is shared over all of the runners, and I wonder if the repeat count could not be messing up when twice the same test is running at the same time.
DeviceWrapperinstance now prefixes its logs with a device identifiertestInfo: TestInfoa mandatory parameter for all tests