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

Finish launches and merges #135

Open
thomaswinkler opened this issue Nov 23, 2022 · 23 comments
Open

Finish launches and merges #135

thomaswinkler opened this issue Nov 23, 2022 · 23 comments

Comments

@thomaswinkler
Copy link
Contributor

thomaswinkler commented Nov 23, 2022

We've been struggling with unfinished launches and merge timeouts in ReportPortal. Unfinished launches, also reported in other issues (such as #62, #63, #64, #73, #79, #133, ...) , for us, especially did come up for suites with number of test greater than 10-20. Smaller numbers appear flaky, but finish at least most of the times.

Root cause appears to be cypress-io/cypress#7139.

As agent-js-cypress is using an async architecture, it also suffers from Cypress killing IPC subprocess before it finishes pushing all data into ReportPortal. IPC messages come in very slow, so depending on the number of events / messages, pushing all changes to ReportPortal takes much longer than Cypress keeps browser runtime open.

The following workaround on Cypress 10+ did fix upload and merge for us. As I do not think the issue can be easily fixed in agent-js-cypress, I would recommend to try the workaround and document. This requires isLaunchMergeRequired to be true. Merging itself could also be disabled in after:run depending on project needs, but to find out when agent-js-cypress is finished, the rplaunchinprogress temp files are needed.

cypress.config.ts:

const delay = async (ms: number) => new Promise((res) => setTimeout(res, ms));

const reportportalOptions = {
  endpoint: 'https://reportportal.abc.io/api/v1',
  isLaunchMergeRequired: true,
  debug: false,
  restClientConfig: {
    timeout: 360000,
  },
  ...
};

export default defineConfig({
  ...
  reporter: 'cypress-multi-reporters',
  reporterOptions: {
    reporterEnabled: '@reportportal/agent-js-cypress, spec',
    reportportalAgentJsCypressReporterOptions: reportportalOptions
  },
  e2e: {
    setupNodeEvents(on, config) {
      // keep Cypress running until the ReportPortal reporter is finished. this is a
      // very critical step, as otherwise results might not be completely pushed into
      // ReportPortal, resulting in unfinsihed launches and failing merges
      on('after:run', async () => {
        console.log('Wait for reportportal agent to finish...');
        while (glob.sync('rplaunchinprogress*.tmp').length > 0) {
          await delay(2000);
        }
        console.log('reportportal agent finished');
        if (reportportalOptions.isLaunchMergeRequired) {
          try {
            console.log('Merging launches...');
            await mergeLaunches(reportportalOptions);
            console.log('Launches successfully merged!');
            deleteLaunchFiles();
          } catch (mergeError: unknown) {
            console.error(mergeError);
          }
        }
      });
      registerReportPortalPlugin(on, config);
      return config;
    },
    ...
    baseUrl: 'http://localhost:9000',
  },
});

Also be aware of an issue in @reportportal/client-javascript not merging more than 20 launches.
reportportal/client-javascript#86
reportportal/client-javascript#103

@zlareb1-yb
Copy link

@thomaswinkler is this workaround working only on Cypress 10+

@thomaswinkler
Copy link
Contributor Author

@thomaswinkler is this workaround working only on Cypress 10+

The workaround is based on setupNodeEvents(on, config) introduced with Cypress 10. You can try with Cypress 9 by implementing in cypress/plugins/index.js. Important is to use on('after:run', ...) to keep Cypress running until all launches are merged.

@gustawx
Copy link

gustawx commented Dec 13, 2022

@thomaswinkler could you please include imports? I can't find deleteLaunchFiles().
Additionally, I found mergeLaunches in @reportportal\agent-js-cypress\lib\ is this the correct one?

Thank you!

@thomaswinkler
Copy link
Contributor Author

thomaswinkler commented Dec 13, 2022

@thomaswinkler could you please include imports? I can't find deleteLaunchFiles(). Additionally, I found mergeLaunches in @reportportal\agent-js-cypress\lib\ is this the correct one?

@gustawx Yes, mergeLaunches can be used from @reportportal\agent-js-cypress\lib\, but I am using a slighly customized version in my cypress.config.ts to fix merge more than 20 launches issue. See #103 for required change.

You can implement deleteLaunchFiles in cypress.config.ts. It's just used to delete all rplaunch*.tmp files before suite launch. Implementation is taken from agent-js-cypress.

const fs = require("fs");
const glob = require("glob");

function deleteLaunchFiles() {
  const getLaunchTempFiles = () => {
    return glob.sync("rplaunch*.tmp");
  };
  const deleteTempFile = (filename) => {
    fs.unlinkSync(filename);
  };
  const files = getLaunchTempFiles();
  files.forEach(deleteTempFile);
}

Let me know if it works. Launching and merging is now very reliable in our test suites. Unfortunately it's just a lot of bugs in required components and even bugfixes (as #103) are not merged. Do have a rather complex setup and configuration now, working around, fixing and adding features to get all infos into Report Portal we need.

@gustawx
Copy link

gustawx commented Dec 15, 2022

@thomaswinkler thank you very much, it works fine now but I found another issue: tests marked as .skip() are never finished in RP and need to be stopped manually. I will file a seperate defect for this.
In such case your solution gets in to infinite loop in while (glob.sync('rplaunchinprogress*.tmp').length > 0)... . You need to break it after some specified number of repeats of the loop

@thomaswinkler
Copy link
Contributor Author

thomaswinkler commented Dec 16, 2022

@gustawx skipping is one of the many bugs I also ran into. Assume with skip you are referring to it.skip, etc. and not the setStatusSkipped custom command, right? Did not use setStatusSkipped myself however, so can only confirm this to be broken for it.skip, describe.skip and context.skip.

Also see https://docs.cypress.io/guides/core-concepts/writing-and-organizing-tests#Pending for infos on Pending state. There is a difference between Pending and Skipped.

Fixing Pending tests does require changes in agent-js-cypress. Not aware of any workaround. I guess you need to add listener for EVENT_TEST_PENDING to deal with skipped tests. Currently only TEST_BEGIN and TEST_END are supported. Should be easy to fix, so if needed I could look into it.

@gustawx
Copy link

gustawx commented Dec 17, 2022

@thomaswinkler thank you very much for your help. skipping is one of the many bugs I also ran into exactly, same here.
The one that is really confusing for me is that RP custom commands don't work, like they would have not been imported. I get simply error that command doesn't exists in cy... Did you do anything special to make them work? I imported the custom commands in my e2e.ts file (like in Readme). The only one that works for me is cy.log() => info appears in the RP results, so this is super confusing that the rest doesn't work.

@thomaswinkler
Copy link
Contributor Author

If anyone interested, I could prepare a pull request for fixing skipped (pending) tests.

@gustawx re: your question about custom commands. Fixing might just require declaring the report portal commands like you need to do for all custom commands in Cypress. agent-js-cypress does not include a typescript d.ts definition for cypresscommands.js.

See Types-for-Custom-Commands documentation for details. Using one myself and no issues at all with custom commands.

@ivan-stratify
Copy link

ivan-stratify commented Jan 3, 2023

If anyone interested, I could prepare a pull request for fixing skipped (pending) tests.

@thomaswinkler: yes please, PR with pending/skipped tests fix would be very helpful. Skipped tests are causing a lot of mess in test runs.

@thomaswinkler
Copy link
Contributor Author

@ivan-stratify, @gustawx I just created #141 to support finishing of skipped tests. Would be great if you could test and confirm it works for you.

@gustawx
Copy link

gustawx commented Jan 11, 2023

@thomaswinkler tested - all works fine, great job! Who's going to merge it?

@thomaswinkler
Copy link
Contributor Author

@gustawx, not sure. Seems not like anyone working on a release. Also did not receive any feedback to my proposed workarounds from maintainers so far. Would now continue to create few more pull requests on other issues. Maybe that will get a conversation and eventually a new release going.

@emanuelradac
Copy link

emanuelradac commented Jan 11, 2023

Yes, mergeLaunches can be used from @reportportal\agent-js-cypress\lib\, but I am using a slighly customized version in my cypress.config.ts to fix merge more than 20 launches issue. See #103 for required change.

Hi @thomaswinkler, you said that you use a customized version for mergeLaunches. Can share your version here? I tried to make a workaround for the problem with merge more than 20 launches, but it didn't work.Thank you!

I'm using Cypress 8.5, but I managed to implement the solution for unfinished launches and merge timeouts as you suggested above in cypress/plugins/index.js and it works. Thank for that!

@gustawx
Copy link

gustawx commented Jan 12, 2023

@AmsterGet @ElenaRomanchuk it looks like you are an active @epam contributors in this repository. Is anyone from epam still maintaing this repo? Can someone take care of the PR raised by @thomaswinkler . There is also a very easy fix proposed for issue related to failing to merge more than 20 results...

@AmsterGet
Copy link
Member

Hello guys!
Thank you very much for your patience!
At the moment we are in progress of reorganizing our team capacity to pay more attention to this repo and to take care of this issue.
So for now I guess we will accept the suggested workaround from @thomaswinkler and update our readme with related info and may be add this to the plugin functionality, I'll release a patch version. As the removing dependency from node-ipc and building the new approach to deal with parallel executions in Cypress time consuming, we have it in the roadmap, but I guess it will be reworked in the scope of the next major version, which will be linked to the new major version of RP.

@thomaswinkler huge thanks for your contribution! I'll try to review and publish your changes this/next week!
@gustawx I guess the suggested fix related to reportportal/client-javascript#86 is acceptable, the same answer here, I plan to deal with it this/next week.

Feel free to ping me in case of the delay from my side or other asks.

@gustawx
Copy link

gustawx commented Jan 12, 2023

@AmsterGet thank you for your answer. I guess the suggested fix related to https://github.com/reportportal/client-javascript/issues/86 is acceptable - yes it is. I haven't tested it yet with my repo where I have 2k tests but at least it's going to work in >90% of cases. At the moment it works for me for set of 150 tests

@thomaswinkler
Copy link
Contributor Author

thomaswinkler commented Jan 12, 2023

@AmsterGet Thank you for your feedback. Please let me create a few more PRs first. There is at least some changes required to improve reliability of the suggested workaround. It's basically about error handling and avoiding the wait to never finish.

Additionally I would like to suggest a new approach for finding screenshots based on after:screenshot plugin event instead of current glob patterns. Will create a PR for your review and feedback.

I could also support moving the workaround to plugin.

@thomaswinkler
Copy link
Contributor Author

@AmsterGet created my pull requests. Not all related to this issue only, but issues I ran into over last couple month.

@gustawx would be interested in your feedback as well on #146 and #148.

@thomaswinkler
Copy link
Contributor Author

@AmsterGet For now, I added pull requests for all issues we ran into or features we required. Please note, I tried to separate into different pull requests what we are using already in production. If you need, I can point you at some branch that has everything merged.

I would next look into creating a pull request for keeping the plugin running until merge is finished. Ok?

@gustawx
Copy link

gustawx commented Feb 17, 2023

hello @thomaswinkler sorry for such a late reponse but I was overloaded with work and changed my work focus on a different subject. I'm coming back to this now and want to move it forward. Could you please resolve the merge conflict #146 ? I will, do the review and test this next week.

btw. great job @thomaswinkler !

@AmsterGet will my review and testing be enough for you to merge the PRs?

@thomaswinkler
Copy link
Contributor Author

@gustawx Fixed merge conflicts in #146.

@AmsterGet
Copy link
Member

Hello guys!
@thomaswinkler thank you for all your work, finally I found a time to dive dipper into your suggestions - I'll leave some questions and thoughts regarding PRs.
@gustawx your help is really appreciated, in addition to this I want to review the changes anyway to better understand improvements and may be align them with my vision.

@StephenHuynh
Copy link

Hi @thomaswinkler
I'm getting an issue with autoMerge, when running tests with Report Portal in Docker on my local machine. When I tried to config isLaunchMergeRequired = true as you suggested, then it works perfectly.
Then, I tried to run same tests with the same Cypress configuration but with different RP setup (I used Azure and deploy RP docker on Azure). I do get some launches on new RP, but after execution done, the launches were not merged.
Do you have any idea for this issue ?

My Env:
@reportportal/agent-js-cypress@5.1.2
cypress@12.10.0

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

No branches or pull requests

7 participants