Fix VideoPress block test cases by fluiddot · Pull Request #5916 · wordpress-mobile/gutenberg-mobile

fluiddot · 2023-06-29T15:26:36Z

This PR aims to fix the failures in #5874 related to unit tests.

⚠️ It's expected that some of the CI jobs fails due to the RN upgrade. The jobs that this PR aims to fix are:

ci/circleci: Test Android
ci/circleci: Test iOS

We could omit the rest as they will be solved in other PRs.

To test:

Run the command: npm run test.
Observe that all tests pass.

Alternatively, we could rely on the CI job that runs the unit tests.

PR submission checklist:

I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

Instead of using a constant for the block HTML, now we generate the HTML via the `generateBlockHTML` util.

This is needed to keep the same block attributes set via the block HTML generation. Extract fetch mock metadata to a constant

The only change is that the video is now public when setting a caption. This has no impact in the testing logic.

fluiddot · 2023-06-29T15:47:00Z

src/test/videopress/edit.js

+	post_id: 1,
+	duration: 2803,


These parameters are not really being tested but decided to include them to avoid updating the Jest snapshots. Especially, as updating snapshots usually makes the changes harder to review.

I have no objections to reviewing a snapshot change if it means the stored snapshot is more helpful or realistic. However, if these mocked parameters achieve a more realistic picture, then it makes sense to keep them IMO.

To be honest, these parameters don't provide any value in the snapshot. They aren't meant to be updated in the test cases but since the VideoPress block updates settings automatically from the API when rendering the block, they are included. I'm happy with both options, but if including them in the tests gives a wrong impression of their usage, I can revert it.

I have no strong preference. I suppose I mostly wanted to convey that we should not be afraid of updating snapshots. If a given snapshot is too unwieldily to review, a more appropriate alternative may be forgoing snapshots altogether and asserting against specific outcomes, e.g. attributes within the snapshot.

I have no strong preference. I suppose I mostly wanted to convey that we should not be afraid of updating snapshots.
Yep, I think in this case we can update the snapshots as we don't have that many to make the review complex 👍.

If a given snapshot is too unwieldily to review, a more appropriate alternative may be forgoing snapshots altogether and asserting against specific outcomes, e.g. attributes within the snapshot.

Good point. In general, I'd rather lean on snapshots to simplify test logic. But I agree that in some cases there's a good value on asserting specific outcomes. For these test cases, I'm hesitant about which one would fit better. For now, I'd prefer to keep them as-is and only focus on the fixes, if that makes sense.

For now, I'd prefer to keep them as-is and only focus on the fixes, if that makes sense.

Absolutely. 🚀

I've just applied this suggestion via 68a1ccd.

fluiddot · 2023-06-29T15:49:59Z

src/test/videopress/local-helpers/utils.js

+		{
+			request: {
+				path: `/wpcom/v2/videopress/${ guid }/check-ownership/${ postID }`,
+				method: 'GET',
+			},
+			response: {
+				'video-belong-to-site': belongsToSite,
+			},
+		},


We need to mock this response, otherwise, some of the block settings like the title will be locked.

dcalhoun · 2023-06-29T15:55:15Z

src/test/videopress/edit.js

+	post_id: 1,
+	duration: 2803,


I have no objections to reviewing a snapshot change if it means the stored snapshot is more helpful or realistic. However, if these mocked parameters achieve a more realistic picture, then it makes sense to keep them IMO.

src/test/videopress/local-helpers/utils.js

Fixes the JSDoc type of `isVideoPrivate` and `isSitePrivate` params. Co-authored-by: David Calhoun <github@davidcalhoun.me>

peril-wordpress-mobile · 2023-06-29T16:11:39Z

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

fluiddot · 2023-06-29T16:53:32Z

src/test/videopress/__snapshots__/edit.js.snap

 // Jest Snapshot v1, https://goo.gl/fbAQLP

-exports[`Update VideoPress block's settings should update Playback Bar Color section's Dynamic color setting 1`] = `"<!-- wp:videopress/video {"title":"default-title-is-file-name","description":"","id":1,"guid":"AbCdEfGh","privacySetting":2,"allowDownload":false,"rating":"G","isPrivate":true,"duration":2803} /-->"`;
+exports[`Update VideoPress block's settings should update Playback Bar Color section's Dynamic color setting 1`] = `"<!-- wp:videopress/video {"title":"default-title-is-file-name","description":"","id":34,"guid":"AbCdEfGh","privacySetting":2,"allowDownload":false,"rating":"G","isPrivate":true,"duration":1200} /-->"`;


The id and duration attributes have been modified due to using the default values specified in the util function (reference). They don't have an impact on the test cases, so we can omit them.

gutenberg-mobile/src/test/videopress/local-helpers/utils.js

Lines 203 to 229 in 68a1ccd

const postID = metadata?.post_id ?? 34;

return [

{

request: {

path: `/wpcom/v2/media/videopress-playback-jwt/${ guid }`,

method: 'POST',

body: {},

},

response: {

metadata_token: token,

},

},

{

request: {

path: isPrivate

? `/rest/v1.1/videos/${ guid }?metadata_token=${ token }`

: `/rest/v1.1/videos/${ guid }`,

credentials: 'omit',

global: true,

},

response: {

description: 'video-description',

post_id: postID,

guid,

private_enabled_for_site: false,

title: 'video-title',

duration: 1200,

fluiddot · 2023-06-29T17:07:24Z

src/test/videopress/local-helpers/utils.js

+	}
+	return {
+		privacySetting: isVideoPrivate ? 1 : 0,
+		isPrivate,


Sorry I didn't notice before 🤦 , but this should be:

isPrivate: isVideoPrivate,

I'll push a fix directly in the base branch (4349216) 💨

fluiddot added 6 commits June 29, 2023 17:24

Add util to generate VideoPress privacy settings for tests

4671e69

Mock VideoPress check ownership API request

f689236

Update VideoPress edit test cases

9ab0d09

Instead of using a constant for the block HTML, now we generate the HTML via the `generateBlockHTML` util.

Use different fetch mocks in VideoPress edit test cases

26bffd4

This is needed to keep the same block attributes set via the block HTML generation. Extract fetch mock metadata to a constant

Update VideoPress description setting placeholder query

5b093ed

Update VideoPress test snapshots

c388815

The only change is that the video is now public when setting a caption. This has no impact in the testing logic.

fluiddot added the Testing Anything related to automated tests label Jun 29, 2023

fluiddot self-assigned this Jun 29, 2023

fluiddot commented Jun 29, 2023

View reviewed changes

fluiddot marked this pull request as ready for review June 29, 2023 15:50

fluiddot requested review from SiobhyB and dcalhoun June 29, 2023 15:50

dcalhoun reviewed Jun 29, 2023

View reviewed changes

Update src/test/videopress/local-helpers/utils.js

124cb96

Fixes the JSDoc type of `isVideoPrivate` and `isSitePrivate` params. Co-authored-by: David Calhoun <github@davidcalhoun.me>

fluiddot added 2 commits June 29, 2023 18:31

Fix value used to generate privacy setting in generatePrivacySettings

2ab22a2

Remove unneeded metadata on fetch mocks

68a1ccd

fluiddot commented Jun 29, 2023

View reviewed changes

dcalhoun approved these changes Jun 29, 2023

View reviewed changes

fluiddot merged commit 4662b08 into upgrade/react-native-0.71.8 Jun 29, 2023

fluiddot deleted the fix/videopress-block-tests branch June 29, 2023 16:57

fluiddot commented Jun 29, 2023

View reviewed changes

	const postID = metadata?.post_id ?? 34;
	return [
	{
	request: {
	path: `/wpcom/v2/media/videopress-playback-jwt/${ guid }`,
	method: 'POST',
	body: {},
	},
	response: {
	metadata_token: token,
	},
	},
	{
	request: {
	path: isPrivate
	? `/rest/v1.1/videos/${ guid }?metadata_token=${ token }`
	: `/rest/v1.1/videos/${ guid }`,
	credentials: 'omit',
	global: true,
	},
	response: {
	description: 'video-description',
	post_id: postID,
	guid,
	private_enabled_for_site: false,
	title: 'video-title',
	duration: 1200,

Conversation

fluiddot commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fluiddot Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fluiddot commented Jun 29, 2023 •

edited

Loading

peril-wordpress-mobile bot commented Jun 29, 2023 •

edited

Loading

fluiddot Jun 29, 2023 •

edited

Loading