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

Fix: e2e #112

Open
wants to merge 16 commits into
base: development
Choose a base branch
from
Open

Fix: e2e #112

wants to merge 16 commits into from

Conversation

nick-dev123
Copy link

@nick-dev123 nick-dev123 commented Oct 8, 2024 •
edited
Loading

Resolves #109

Removes obsolete tests:

  • Should display retry time frame with no tasks loaded and a logged in user
    • Rate limiting cannot be triggered by logged in user
  • should display an error modal when fetching issue previews fails on page load
    • API call has been removed

New tests:

  • Main page shouldn't display invalid issue

@nick-dev123 nick-dev123 marked this pull request as ready for review October 9, 2024 12:45
@nick-dev123 nick-dev123 changed the title [WIP]Fix: e2e Fix: e2e Oct 9, 2024
@0x4007
Copy link
Member

0x4007 commented Oct 10, 2024

There's one failing still

@0x4007 0x4007 marked this pull request as draft October 10, 2024 06:15
@nick-dev123
Copy link
Author

@0x4007 Fixed the failing test.

@rndquu rndquu self-requested a review October 11, 2024 09:07
@rndquu rndquu assigned rndquu and unassigned rndquu Oct 11, 2024
@rndquu rndquu marked this pull request as ready for review October 11, 2024 09:07
@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Oct 11, 2024 •
edited
Loading

rndquu
rndquu requested changes Oct 11, 2024
View reviewed changes
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-10-11 at 12 28 08

CSS is broken, pls fix

@rndquu rndquu marked this pull request as draft October 11, 2024 09:29
@nick-dev123 nick-dev123 marked this pull request as ready for review October 12, 2024 04:00
0x4007
0x4007 approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Looks fine on CD and CI is green.

@gentlementlegen
Copy link
Member

gentlementlegen commented Oct 15, 2024 •
edited
Loading

I do not know if that's introduced by this pull request or because I test locally, but when I log in I have this in my app bar
image

I do not see this on the production version.

I reverted the changes and tested, the issue does not happen so I believe this fix introduces that.

@nick-dev123
Copy link
Author

@gentlementlegen The lock icon is called augment-access-button and as far as I can tell it allows users to access private repositories related tasks.
The Should display filters on small devices test case had the following requirement:
cy.get("#augment-access-button").should("be.visible");
I can remove it if it's not required.

@gentlementlegen
Copy link
Member

@nick-dev123 I meant that the "login button" should not be there, as I am logged in. Do you have this happening as well?

@rndquu rndquu self-requested a review October 18, 2024 14:45
@rndquu
Copy link
Member

rndquu commented Oct 18, 2024

@nick-dev123 I meant that the "login button" should not be there, as I am logged in. Do you have this happening as well?

Can't not reproduce, the login button is hidden when I'm logged in.

@nick-dev123 Could you merge the latest development branch?

@nick-dev123
Copy link
Author

@rndquu done

0x4007
0x4007 reviewed Oct 23, 2024
View reviewed changes
src/home/rendering/display-github-user-information.ts
@@ -37,12 +37,14 @@ export async function displayGitHubUserInformation(gitHubUser: GitHubUser) {
window.location.reload();
});

containerDivElement.appendChild(authenticatedDivElement);
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct to append? I expected only changes to tests.

Copy link
Member

Choose a reason for hiding this comment

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

@nick-dev123 Why was the UI updated?

@ubiquity-os ubiquity-os
Copy link
Contributor

ubiquity-os bot commented Nov 4, 2024

@nick-dev123, this task has been idle for a while. Please provide an update.

1 similar comment
@ubiquity-os ubiquity-os
Copy link
Contributor

ubiquity-os bot commented Nov 12, 2024

@nick-dev123, this task has been idle for a while. Please provide an update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0x4007 0x4007 0x4007 approved these changes

@gentlementlegen gentlementlegen Awaiting requested review from gentlementlegen

@rndquu rndquu Awaiting requested review from rndquu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ci: fix cypress e2e tests
4 participants
@nick-dev123 @0x4007 @gentlementlegen @rndquu