agent-js-cypress icon indicating copy to clipboard operation
agent-js-cypress copied to clipboard

Finish launches and merges

Open thomaswinkler opened this issue 2 years ago • 23 comments

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 https://github.com/cypress-io/cypress/issues/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. https://github.com/reportportal/client-javascript/issues/86 https://github.com/reportportal/client-javascript/pull/103

thomaswinkler avatar Nov 23 '22 10:11 thomaswinkler

@thomaswinkler is this workaround working only on Cypress 10+

zlareb1-yb avatar Dec 07 '22 13:12 zlareb1-yb

@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.

thomaswinkler avatar Dec 07 '22 20:12 thomaswinkler

@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!

gustawx avatar Dec 13 '22 13:12 gustawx

@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.

thomaswinkler avatar Dec 13 '22 18:12 thomaswinkler

@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

gustawx avatar Dec 15 '22 07:12 gustawx

@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.

thomaswinkler avatar Dec 16 '22 17:12 thomaswinkler

@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.

gustawx avatar Dec 17 '22 13:12 gustawx

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.

thomaswinkler avatar Dec 18 '22 19:12 thomaswinkler

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.

ivan-stratify avatar Jan 03 '23 08:01 ivan-stratify

@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.

thomaswinkler avatar Jan 11 '23 10:01 thomaswinkler

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

gustawx avatar Jan 11 '23 14:01 gustawx

@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.

thomaswinkler avatar Jan 11 '23 19:01 thomaswinkler

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!

emanuelradac avatar Jan 11 '23 22:01 emanuelradac

@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...

gustawx avatar Jan 12 '23 07:01 gustawx

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 https://github.com/reportportal/client-javascript/issues/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.

AmsterGet avatar Jan 12 '23 11:01 AmsterGet

@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

gustawx avatar Jan 12 '23 12:01 gustawx

@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 avatar Jan 12 '23 19:01 thomaswinkler

@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 avatar Jan 15 '23 16:01 thomaswinkler

@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?

thomaswinkler avatar Jan 26 '23 08:01 thomaswinkler

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?

gustawx avatar Feb 17 '23 09:02 gustawx

@gustawx Fixed merge conflicts in #146.

thomaswinkler avatar Feb 18 '23 19:02 thomaswinkler

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.

AmsterGet avatar Feb 20 '23 16:02 AmsterGet

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/[email protected] [email protected]

StephenHuynh avatar Apr 27 '23 11:04 StephenHuynh