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

[CY] Add template and fix vm #359

Merged
merged 6 commits into from
Jul 6, 2022
Merged

Conversation

n313893254
Copy link
Contributor

@n313893254 n313893254 commented May 31, 2022

Changes:

  • Add template case: "Create a VM template with all the required values"
  • Regression VM test case and fix it.

@n313893254 n313893254 marked this pull request as ready for review May 31, 2022 10:00
@n313893254 n313893254 requested a review from DaiYuzeng June 1, 2022 02:09
Copy link
Contributor

@TachunLin TachunLin left a comment

Choose a reason for hiding this comment

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

  • Verify the create vm template test cases in template.spec running correctly
    image

  • Verify the images spec test cases failed in the following tests

    • Create an image with valid image URL
    • Delete VM with exported image
    • Update image labels after deleting source VM
      image
  • In the create an image with url, suggest not to check the file size for the daily build, since the file size may be different in each day
    image

  • Failure log of Delete VM with exported image
    image

  • Failure log of Update image labels after deleting source VM
    image

@n313893254
Copy link
Contributor Author

@TachunLin Thanks for your review.

  • Remove size check
  • Add auto-setup case for creating VM
  • Fix add image label index

@n313893254 n313893254 requested a review from TachunLin June 10, 2022 09:07
Copy link
Contributor

@TachunLin TachunLin left a comment

Choose a reason for hiding this comment

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

Verified on commit [8585d05] (8585d05)
on newly installed one node harvester.

  • The test Auto setup image from cypress environment got assertion error
Check Image list: expected '' to equal 200

At: [testcases/image/images.spec.ts:31:50]

image

It looks like may related to page synchronization of loading latency.

@n313893254 n313893254 requested a review from TachunLin June 14, 2022 08:35
Copy link
Contributor

@TachunLin TachunLin left a comment

Choose a reason for hiding this comment

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

Verified on commit 4a1552d

  • The first image test case Auto setup image from cypress environment would failed
    cypress can find page contains the image name
    image
    image

  • The first image test case failed would also affect the second test Create an image with valid image URL
    image
    image

  • Then the Cypress test runner will broken
    image

  • The actual image download status on Harvester image page (From another browser)
    image

It looks like the first test case failed would affect the upcoming test execution result.

@noahgildersleeve
Copy link
Contributor

@n313893254 We did an update to the Cypress version, so you probably want to do a pull from main to your PR. I don't think it should affect your tests very much since it was mostly a change to the config JSON and the version of Cypress (9.3 -> 10.2.0). Please let me know if it caused any issues.

@noahgildersleeve
Copy link
Contributor

Also if you change the ISO link in the config to https://mirrors.bfsu.edu.cn/opensuse/distribution/leap/15.3/iso/openSUSE-Leap-15.3-3-NET-x86_64-Media.iso you will probably get less timeouts since it's only 147MB instead of 4GB. I reran some of the tests that way and it fixed a lot of the timeout based problems.

@n313893254
Copy link
Contributor Author

n313893254 commented Jun 29, 2022

@TachunLin, Thanks for your review.

  • Set numTestsKeptInMemory to 0 to solve the browser crash.
  • Per issue, set defaultCommandTimeout to 1 min to solve the Dom can not be found before the UI render.
  • Fix the image name contains uppercase
  • Remove goToYamlDetail because the detail page removes the YAML button.

@noahgildersleeve
Copy link
Contributor

noahgildersleeve commented Jun 29, 2022

I'm getting a failure when running the tests. It's failing
Update image labels after deleting source VM
1) Update image labels after deleting source VM

Screenshot_20220629_162022
Screenshot_20220629_162012

@n313893254
Copy link
Contributor Author

n313893254 commented Jun 30, 2022

@noahgildersleeve . Thanks for your review. Delete VM will change the image resource version. Per dashboard code fix the 409 conflicts.

@n313893254 n313893254 reopened this Jun 30, 2022
@noahgildersleeve
Copy link
Contributor

All image tests passed for me after fix on harvester master-949bbb0c-head.
Screenshot_20220630_143652

@noahgildersleeve
Copy link
Contributor

I checked the other tests that were updated and they are failing. There was a 409 response code error, a typescript error, a difference in an assertion on memory size, and a timeout error)
Screenshot_20220630_161132
Screenshot_20220630_161530
Screenshot_20220630_161514
Screenshot_20220630_161455
Screenshot_20220630_161436

@n313893254 n313893254 marked this pull request as draft July 1, 2022 06:21
@n313893254 n313893254 marked this pull request as ready for review July 1, 2022 08:42
@n313893254
Copy link
Contributor Author

@noahgildersleeve Thank you. Fixed and there have two changes:

  • The tested UI should use v1.0.3 latest UI. Set dashboard-ui-index to https://releases.rancher.com/harvester-ui/dashboard/release-harvester-v1.0/index.html.
  • Network requires two VLANs in cypress.config.ts.

Copy link
Contributor

@noahgildersleeve noahgildersleeve left a comment

Choose a reason for hiding this comment

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

LGTM. All associated specs are passing with the 1.0.3 ui-index
Screenshot_20220705_163450

@noahgildersleeve
Copy link
Contributor

@TachunLin I think you need to approve the changes before this PR can be merged.

@TachunLin
Copy link
Contributor

TachunLin commented Jul 6, 2022

Thanks @noahgildersleeve, really appreciated for checking these tests.

@noahgildersleeve noahgildersleeve merged commit 2ff6ce6 into harvester:main Jul 6, 2022
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.

3 participants