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.

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

UPSE-386: Update Playwright search tests for 'favorite' flag #648

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

groybal
Copy link
Contributor

@groybal groybal commented Aug 1, 2023

Checklist
Description of change

For my uPortal PR (uPortal-Project/uPortal#2678) for UPSE-386, it was requested that I update the Playwright scripts to account for the 'favorite' flag added to the portlet results. This PR does that.

This PR should not be merged until the uPortal UPSE-386 PR is merged.

@sonatype-lift
Copy link

sonatype-lift bot commented Aug 1, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

Copy link
Contributor

@cbeach47 cbeach47 left a comment

Choose a reason for hiding this comment

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

Minor inline comment.

Overall, LTGM - thanks @groybal !

);
expect(response.status()).toEqual(200);
const responseBody: PreferenceFavoriteResponse = JSON.parse(await response.text()) as PreferenceFavoriteResponse;
return responseBody.newNodeId;//response.startsWith('Added');
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we could remove this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, I'll remove

@cbeach47
Copy link
Contributor

cbeach47 commented Aug 1, 2023

@groybal - for

This PR should not be merged until the uPortal UPSE-386 PR is merged.

This PR should actually not be merged until that PR is merged AND a release is cut. Current phase of these playwright tests work off of the uPortal-start configs, which are the releases for the core uPortal and portlet services. I'd like to see how we can enhance this flow so the playwright tests 'go with' the code change for a given repo.

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @groybal! LGTM
Added some nice to have ideas for the PR.

const response = await request.get(`${config.url}api/v5-0/portal/search?q=cartoon&type=portlets`);
expect(await unfavoritePortlet(request, portletId)).toBe(true);
expect(response.status()).toEqual(200);
const portletSearchResults: PortletSearchResults = JSON.parse(await response.text()) as PortletSearchResults;
Copy link
Member

Choose a reason for hiding this comment

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

an as cast is probably fine for a test case.
Using a JSON parser/validator like zod would give better assurance that the structure returns matches the test expectation. https://zod.dev/

): Promise<PortletDefBasicInfo> {
const response = await request.get(`${config.url}api/portlets.json`);
expect(response.status()).toEqual(200);
const portletsJson: PortletsResponse = await response.json() as PortletsResponse;
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice if we parsed/validated the JSON.

`${config.url}api/layout?action=addFavorite&channelId=${portletId}`
);
expect(response.status()).toEqual(200);
const responseBody: PreferenceFavoriteResponse = JSON.parse(await response.text()) as PreferenceFavoriteResponse;
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice if we parsed/validated the JSON.

expect(portletId).not.toBeNull();
const response = await request.post(`${config.url}api/layout?action=removeFavorite&channelId=${portletId}`);
expect(response.status()).toEqual(200);
const responseBody: PreferenceFavoriteResponse = JSON.parse(await response.text()) as PreferenceFavoriteResponse;
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice if we parsed/validated the JSON.

Comment on lines +13 to +25
interface PortletSearchResult {
description: string;
fname: string;
name: string;
score: string;
title: string;
url: string;
favorite: boolean;
}

interface PortletSearchResults {
portlets: PortletSearchResult[];
}
Copy link
Member

Choose a reason for hiding this comment

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

Completely optional, but if you are interested in trying Zod.
This would look something like:

import z from 'zod';

const PortletSearchResultSchema = z.object({
  description: z.string(),
  fname: z.string(),
  name: z.string(),
  score: z.string(),
  title: z.string(),
  url: z.string(),
  favorite: z.boolean();
});
type PortletSearchResult = z.infer<typeof PortletSearchResultSchema>;

const PortletSearchResultsSchema = z.object({
  portlets: z.array(PortletSearchResultSchema)
});
type PortletSearchResults = z.infer<typeof PortletSearchResultsSchema>

this could later be used:

const portletSearchResults: PortletSearchResults = PortletSearchResultsSchema.parse(JSON.parse(await response.text()));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants