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

fix(html_report): add if statement #17146

Merged
merged 1 commit into from
Jul 1, 2024
Merged

fix(html_report): add if statement #17146

merged 1 commit into from
Jul 1, 2024

Conversation

ashrivastava-qa
Copy link
Contributor

@ashrivastava-qa ashrivastava-qa commented Jun 18, 2024

Because

  • The reports were not generating for parallel machines that had failed tests runs.

This pull request

  • Added the when parameter to config.yaml file for the 'rename reports' step
  • Added condition to rename files when the reports otherwise provide an apt message
  • Added if condition for merging the reports when the report exist otherwise provide an apt message

Issue that this pull request solves

Closes: FXA-9885 FXA-9979

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

@Trinaa
Copy link
Contributor

Trinaa commented Jun 20, 2024

Not sure if these things should be in scope in this fix, but we should probably consider the following:

  1. The HTML report doesn't get generated in the case where the functional tests fail:
    image

  2. CircleCI documentation seems to indicate that there are 'cost implications' to storing artifacts. I wonder if we should omit artifacting the blobs and just upload the report? Or at least stop uploading it three times ; In Functional Tests - Playwright we upload the original blobs and the renamed blobs and in Merge Playwright Reports we upload the merged blob-report/report.zip, but it's all the same data. The traces are also getting duplicated.

image image

cc: @pdehaan

@ashrivastava-qa
Copy link
Contributor Author

I am thinking we should create a separate ticket for the things you have mentioned @Trinaa and merge this change so that it unblocks the PRs and the use of 're-run from failed' option. What do you think @pdehaan ?

@ashrivastava-qa
Copy link
Contributor Author

ashrivastava-qa commented Jun 21, 2024

@Trinaa Yes I can see and try to minimize the upload. Also we retain the data for 90 days only due to CircleCI limitation and the doc mentions "Retaining an artifact for a long period of time will have storage cost implications, therefore, it is best to determine why you are retaining artifacts" but I can look into it more to see what the cost looks like.

@ashrivastava-qa
Copy link
Contributor Author

ashrivastava-qa commented Jun 25, 2024

@Trinaa and @pdehaan I created a separate issue to address both the above mentioned items Kat pointed out. I think if this looks okay we can merge this for now to unblock the team and I am working on the other ticket separately.

.circleci/config.yml Outdated Show resolved Hide resolved
@ashrivastava-qa
Copy link
Contributor Author

ashrivastava-qa commented Jun 28, 2024

@Trinaa @pdehaan

  1. I was able to fix the script so that report-[0-7].zip generates even for the failed test machine.
  2. For minimizing the artifacts upload, we are uploading the report.zip file thrice (twice in Functional-Test Playwright job and once in Merge Reports job). However the problem I am encountering when trying to minimize are:

a. If I remove the blob report directory path from Playwright.config.ts file, which is duplicating the reports.zip file in all the machines, no reports.zip file generates at all. Couldn't understand why as the Playwright docs suggests its either the path needs to be setup or the Environment variable but somehow we have to use path with or without Env variables.

b. The second upload is just renaming (not copying) the report.zip file to report-[0-7].zip file and store in artifacts/blob-report so that it can be merged.

c. The last upload is the merged file again in artifacts/blob-report for Merge Reports job to be able to use it for the HTML reporting. And this is required can’t not skip this upload.

d. Tried to remove the 'rename reports' step altogether and use PLAYWRIGHT_BLOB_OUTPUT_FILE="report-$(CIRCLE_NODE_INDEX).zip in config.yaml and playwright.config.ts using the output filename but that didn't work either.

So, IMO the last 2 steps are required but the first step could be removed which is the cause of duplication but somehow removing it doesn't generate the reports file at all. And the CircleCI doc states “Retaining an artifact for a long period of time will have storage cost implications, therefore, it is best to determine why you are retaining artifacts” and we only retain the data for 90days so I am guessing it won’t be too costly and I talked to Ben as well and he mentioned in Nimbus/experimenter he has seen the cost to be minimal for artifacts upload as close to .0001 cent or something. But we can't be certain, I will talk to SRE to see if they have any info on this.

So let me know if you have other thoughts on keeping the extra upload happening in step 1.

here's the POC PR

Copy link
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

@ashrivastava-qa Makes sense to me 👍🏽

@ashrivastava-qa ashrivastava-qa merged commit f1d9ace into main Jul 1, 2024
24 checks passed
@ashrivastava-qa ashrivastava-qa deleted the fix-html-report branch July 1, 2024 17:24
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