chore: [NO-JIRA] update lib to latest from upstream #2

jeremiah-snee-openx · 2024-07-03T03:34:49Z

Merge update from upstream.

adds testing for cypress v13
stability improvements

oxscott · 2024-07-03T21:20:07Z

@olga-govor can you please advise on how to publish these changes?

olga-govor · 2024-07-04T08:20:27Z

cypress/e2e/1/delete-pizza.cy.js

@@ -0,0 +1,35 @@
+describe('Delete pizza', () => {


why do we need all that example tests?

This is used for local test validation of cypress plugin. It is not supposed to be included in the published distribution of the package.

ok, but if we decide to keep tests then we need to setup ci/cd for it and also add tests for customisation

olga-govor · 2024-07-04T08:21:50Z

@oxscott I would say first we need to clean up this PR - it contains a lot of unnecessary staff that is presented in parent lib only as an example.

olga-govor

@jeremiah-snee-openx we won't merge this as this edition contains a lot of code that we don't use and we won't use - it's an example in parent repo. If we need to update cypress version let's update cypress version, but do not change name of package, etc.

Also if we want to update with original changes in libs and also do some functional changes - it should be splitted on separate PR's - as for now it's not understandable what is from parent lib and what was updated by you and why?

Changes in this lib will affect UI tests in ui-unity and ui-tests, so we can't do it in such way

And sorry, but changes in lib that is widely used should have JIRA ticket with clear description what do we change and why

olga-govor · 2024-07-04T08:28:12Z

package.json

-  "version": "0.2.3",
-  "description": "Reduce up to 40% your Cypress suite execution time parallelizing the test run on the same machine.",
-  "main": "cli.js",
+  "name": "cypress-parallel-dev",


why do we need to change name of package?

apologies if my question was not clear. we could not find docs on how to contribute/test changes here and i was asking how to publish this so that testing can take place. i presume cypress-parallel-dev was chosen to avoid naming conflicts with cypress-parallel.

if we want to test new package we can locally connect our repo with tests to locally update cypress-parallel plugin, to have brief view if everything is ok.
After if everything is ok, new version of plugin can be published (https://docs.npmjs.com/creating-and-publishing-private-packages), but it would be good to have

"@openx/cypress-parallel": "0.2.3"

instead of "@openx/cypress-parallel": "^0.2.3", so it won't be updated by somebody unintentionally

Switch test branch in ui-unity to new version of plugin and run the whole regression.

oxscott · 2024-07-08T17:03:56Z

@olga-govor why wouldn't we just keep this fork in sync with upstream and layer on our customization? wouldn't that be easier than picking and choosing specific updates to reimplement?

edit: or even better would be to drop the fork and contribute directly to upstream with our customizations since it just looks like some additional settings for weights files and timeouts. out of scope for this pr, but something to consider in the future.

olga-govor · 2024-07-09T07:33:14Z

@oxscott if you will take a look at package itself https://www.npmjs.com/package/cypress-parallel?activeTab=code it contains just code from lib folder. Also lib folder organise as separate project (it's own package.json, etc) and actually it's cypress-parallel plugin, so if we want to be up-to date with up-stream we need to sync only one lib folder.

Regarding contribute to open-source library - it's good idea, but it will take time and need separate capacity on it. Also we need to clarify legal part of commiting our work to open-source (do OpenX allow it)

olga-govor · 2024-07-09T11:25:21Z

lib/cli.js

@@ -37,7 +43,7 @@ async function start() {
  const end = new Date();
  const timeTaken = end.getTime() - start.getTime();

-  const resultsPath = path.join(process.cwd(), settings.runnerResults);
+  console.log(`Reading results from ${resultsPath}`);


both in upstream and in master we create variable resultsPath and use it further, why you decided to change this logic ?

olga-govor · 2024-07-09T11:26:11Z

lib/cli.js

@@ -6,7 +6,11 @@ const path = require('path');
 const fs = require('fs-extra');
 const { settings } = require('./settings');

-const { getTestSuitePaths, distributeTestsByWeight } = require('./test-suites');
+const {


if we want to have easy keeping up-to-date with parent then it's better not to restyle upstream code

olga-govor · 2024-07-09T11:38:00Z

lib/thread.js

@@ -31,12 +31,13 @@ function createReporterOptions(string) {
 }

 function createReporterConfigFile(path) {
-  const reporterEnabled = ['@openx/cypress-parallel/json-stream.reporter.js'];
+  console.log('Creating reporter config file', __dirname);


do we need this console log? it's more difference with upstream that is not necessary

olga-govor · 2024-07-09T11:39:02Z

lib/thread.js

@@ -51,7 +52,7 @@ function createReporterConfigFile(path) {
 }

 function createCommandArguments(thread) {
-  const specFiles = `${thread.list.map(path => globEscape(path)).join(',')}`;
+  const specFiles = `${thread.list.map((path) => globEscape(path)).join(',')}`;


why do need this stylish change?

olga-govor · 2024-07-09T11:40:23Z

lib/thread.js

@@ -80,7 +81,7 @@ async function executeThread(thread, index) {
  const commandArguments = createCommandArguments(thread);

  // staggered start (when executed in container with xvfb ends up having a race condition causing intermittent failures)
-  await sleep(index * 5000);
+  await sleep((index + 1) * 2000);


it was intentional increase as 2sec was insufficient time

olga-govor · 2024-07-09T11:41:20Z

lib/utility.js

-  } catch(e) {
-    console.error(e)
+    console.log('Weights file generated under path: ' + weightPath);
+  } catch (e) {


the same: let's keep style as in upstream to not produce additional difference

olga-govor · 2024-07-09T12:14:40Z

Also as a temporary solution till we decide contribute to open-source or totally separate it and bring tests,ci/cd, etc to life we can use just shell script to update our package with upstream changes

#!/bin/bash

# Fetch updates from the upstream repository
git fetch upstream

# Checkout the specific directory from the upstream repository
git checkout upstream/master -- lib

# Move the contents of the lib directory to the root
mv lib/* .

# Remove the now-empty lib directory
rmdir lib

oxscott · 2024-07-09T14:51:47Z

at this point i think it will be easier to just cut a branch of ui-unity that uses the up-to-date public package. this would allow for testing with an up-to-date cypress-parallel package as it relates to the problem we need to solve immediately (e2e test jobs hanging) while not impacting anything here. the public package was updated to support custom weight file location a couple years ago, and it's been used in classic oa ui for a long time without issues, so it should work fine to determine if it helps with our problem in unity.

if the test is successful, we can decide how to either 1) get this forked codebase up-to-date or 2) just switch unity over to the public package (once stability has been proven). if the test is not successful, then we can leave the tech debt work in this private package for another day while we focus on the primary objective of fixing hanging test jobs.

jeremiah-snee-openx · 2024-07-09T14:53:45Z

@olga-govor The only thing that should be published from this repo is the lib folder. The name of the package is the same.

cypress-parallel/lib/package.json

Lines 2 to 4 in a96200c

    
           "name": "@openx/cypress-parallel", 
        
           "version": "0.3.0", 
        
           "description": "Reduce up to 40% your Cypress suite execution time parallelizing the test run on the same machine.",

The example files in cypress directory are used for testing of the package, but not a part of the distribution.
This is how upstream project publishes as well. https://www.npmjs.com/package/cypress-parallel?activeTab=code

olga-govor · 2024-07-09T15:06:09Z

@jeremiah-snee-openx yes, but it doesn't answer why we need to change name in base project:

it still not aligned with upstream project, as there it is "name": "cypress-parallel-demo"

olga-govor · 2024-07-09T15:14:16Z

@oxscott
about options

get this forked codebase up-to-date - let's do this
just switch unity over to the public package (once stability has been proven).- there are also other adjustments that were done and that fix other problems that just adding of weights

May I ask what is the exact change in upstream that probably fix the problem?

oxscott · 2024-07-09T16:02:39Z

@olga-govor the list of updates/improvements is long. you can read through them here. weights file location setting was added here. staggered thread delay was added here. it's not easy to see what else has changed based on the commits straight to master here in the fork and no references to tickets. if there are other critical differences, please let me know.

given that this plugin handles cypress's paywalled parallelism feature for us, it seemed like a reasonable troubleshooting step to try using the latest version which includes a list of bug fixes and dependency updates, some of which relate to thread management or caching.

i didn't expect it to be so disruptive, so i'm suggesting we test these updates using the public package for now before continuing conversation on this PR.

olga-govor · 2024-07-10T08:31:16Z

@oxscott I asked not about improvements that were done in upstream, but improvements that were done by @jeremiah-snee-openx (because there are some changes).

jeremiah-snee-openx · 2024-07-10T19:37:38Z

Changes not from upstream were to fix issues seen in ci testing pipeline related to changes from OpenX Code changes.

https://github.com/openx/cypress-parallel/actions/runs/9772110184/job/26975965771

Was able to find and resolve cypress hanging issues in oa-ui without these updates. Closing PR.

jeremiah-snee-openx added 24 commits July 2, 2024 22:13

move lib to sub directory

217ebfb

git updates

fe614b1

pizza-demo

8651ec0

add cypress

a262161

update repo files

861625f

npm i

1a2d4a1

version bump

cc238bc

update names

c94279c

revert

395f823

merge change

59ff696

remaining conflicts & misc fixes

65778df

rm

c5b90b4

revert changes

9ca5fcb

redundent

4d7e494

format

30e94f4

local debug

5c46554

package version update

59f48de

cli analytics off

5e75402

only install

9193f46

rm

08d25aa

Merge branch 'chore/test' into chore/updates

06e1646

resultPath

3bddaf9

final tweaks

9f14601

update runner

a96200c

oxscott requested review from olga-govor and a team July 3, 2024 21:18

olga-govor reviewed Jul 4, 2024

View reviewed changes

olga-govor reviewed Jul 9, 2024

View reviewed changes

jeremiah-snee-openx closed this Jul 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: [NO-JIRA] update lib to latest from upstream #2

chore: [NO-JIRA] update lib to latest from upstream #2

jeremiah-snee-openx commented Jul 3, 2024

oxscott commented Jul 3, 2024

olga-govor Jul 4, 2024

jeremiah-snee-openx Jul 9, 2024

olga-govor Jul 9, 2024

olga-govor commented Jul 4, 2024 •

edited

Loading

olga-govor left a comment •

edited

Loading

olga-govor Jul 4, 2024

oxscott Jul 9, 2024

olga-govor Jul 9, 2024

oxscott commented Jul 8, 2024 •

edited

Loading

olga-govor commented Jul 9, 2024

olga-govor Jul 9, 2024

olga-govor Jul 9, 2024

olga-govor Jul 9, 2024

olga-govor Jul 9, 2024

olga-govor Jul 9, 2024

olga-govor Jul 9, 2024

olga-govor commented Jul 9, 2024

oxscott commented Jul 9, 2024

jeremiah-snee-openx commented Jul 9, 2024 •

edited

Loading

olga-govor commented Jul 9, 2024

olga-govor commented Jul 9, 2024

oxscott commented Jul 9, 2024

olga-govor commented Jul 10, 2024

jeremiah-snee-openx commented Jul 10, 2024

chore: [NO-JIRA] update lib to latest from upstream #2

chore: [NO-JIRA] update lib to latest from upstream #2

Conversation

jeremiah-snee-openx commented Jul 3, 2024

oxscott commented Jul 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olga-govor commented Jul 4, 2024 • edited Loading

olga-govor left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxscott commented Jul 8, 2024 • edited Loading

olga-govor commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olga-govor commented Jul 9, 2024

oxscott commented Jul 9, 2024

jeremiah-snee-openx commented Jul 9, 2024 • edited Loading

olga-govor commented Jul 9, 2024

olga-govor commented Jul 9, 2024

oxscott commented Jul 9, 2024

olga-govor commented Jul 10, 2024

jeremiah-snee-openx commented Jul 10, 2024

olga-govor commented Jul 4, 2024 •

edited

Loading

olga-govor left a comment •

edited

Loading

oxscott commented Jul 8, 2024 •

edited

Loading

jeremiah-snee-openx commented Jul 9, 2024 •

edited

Loading