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

#2805 Upgrade Node #2990

Merged
merged 10 commits into from
Oct 29, 2024
Merged

#2805 Upgrade Node #2990

merged 10 commits into from
Oct 29, 2024

Conversation

YuaFox
Copy link
Collaborator

@YuaFox YuaFox commented Aug 22, 2024

Upgrade to node v22 LTS and update libraries

This is an experimental pull request

- Nodejs upgraded to v22 LTS
- Cypress upgraded
Copy link

github-actions bot commented Aug 22, 2024

Java Script Mocha Unit Test Results

268 tests   268 ✅  3s ⏱️
 70 suites    0 💤
  1 files      0 ❌

Results for commit 1acb691.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Aug 22, 2024

Java JUnit Test Results

2 517 tests   2 517 ✅  45s ⏱️
  116 suites      0 💤
  116 files        0 ❌

Results for commit 1acb691.

♻️ This comment has been updated with latest results.

@Limraj
Copy link
Collaborator

Limraj commented Aug 22, 2024

Hi @YuaFox,
before committing you can rebuild locally using task: buildRunDebugProdTestUi;
If this task completed locally, it should also works here.

Remember that we have two vue projects, where each time you change the version you have to remove node_modules in places:

  1. Delete ..\Scada-LTS\scadalts-ui\node_modules ;
  2. Git->Rollback on ..\Scada-LTS\scadalts-ui (lib @min-gb in soruce)
  3. Delete ..\Scada-LTS\WebContent\resources\node_modules
  4. Run task buildRunDebugProdTestUi;

Regards,
Kamil Jarmusik

- Added router to tests
- Updated @vue/test-utils
- Updated package-lock.json
!@min-gb/dist
!@min-gb/public
!@min-gb/src
!@min-gb/*.*
Copy link
Collaborator

@Limraj Limraj Aug 28, 2024

Choose a reason for hiding this comment

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

We don't want it in ignore, this library is used in our code, and since it is no longer available in the remote repository we have moved it to sources, what lies ahead is to integrate this library in our sources:
#2957

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm aware of that, thats why i put the !, to include it in the repo. I changed it to that to ignore the node_modules folder inside @min-gb

Copy link
Collaborator

@Limraj Limraj Aug 29, 2024

Choose a reason for hiding this comment

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

Hi @YuaFox, 'to include it in the repo'?
I mean what did you do because I don't understand...

If you set ignore on this library, git will not track changes in it, we don't want anything in the sources to be ignored by git.

Copy link
Collaborator

@Limraj Limraj Aug 29, 2024

Choose a reason for hiding this comment

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

Okay, I understand, you made it so that it won't be ignored? Great, we need to audit this library, what is it, etc.
Can't we rewrite the functions of this library ourselves.

Copy link
Collaborator

@Limraj Limraj Aug 29, 2024

Choose a reason for hiding this comment

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

From what I can see, this configuration does not block the visibility of changes in min-gb, so there is no need to change it here, unless I am missing something, I just did a test and changed the file: \node_modules@min-gb\vuejs-components\babel. config.js and git sees this change.

Copy link
Collaborator

@Limraj Limraj Sep 2, 2024

Choose a reason for hiding this comment

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

@YuaFox You just need to make sure whether there were problems with them on node 14 as well. If so, they can be corrected in a separate issue
.

Copy link
Collaborator

@Limraj Limraj Sep 5, 2024

Choose a reason for hiding this comment

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

Hi @YuaFox,
Can you confirm that the tests worked similarly on node 14?
If so, I'll take a look at the code and we'll merge.
It is best to create a new issue to fix these tests.

Regards,
Kamil Jarmusik

Copy link
Collaborator

@Limraj Limraj Sep 10, 2024

Choose a reason for hiding this comment

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

Hi @YuaFox,
you still need to update the files:

  • WebContent/resources/package-lock.json,
  • scadalts-ui/package-lock.json;

As follows:

  1. We delete these two lock files;
  2. Delete:
  • WebContent/resources/node_modules,
  • scadalts-ui/node_modules (and rollback @min-gb);
  1. We rebuild the project;

I have these files updated, I can commit it to your branch if you don't mind.

#2805_Upgrade_Node__-_corrected__WebContent_resources_package-lock_json,_scadalts-ui_packa.patch

Regards.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @Limraj ,

If you have these files changed feel free to to commit them here.

The unit tests work as expected, the cypress tests there are some that worked on node 14 but not on node 22. I haven't had time to take a closer look and see if it's just a change in the dom or something similar or if they are real bugs.

Copy link
Collaborator

@Limraj Limraj Sep 18, 2024

Choose a reason for hiding this comment

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

Hi @YuaFox,
usage command:
npx cypress run
and Node version:
image
then:
image

@@ -82,6 +82,7 @@ export function prepareShallowMountWrapper(
vuetify,
store,
i18n,
router,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the justification for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the router is not added to tests, SynopticPanel testing don't work because this.$router is undefined.

https://github.com/SCADA-LTS/Scada-LTS/actions/runs/10505804935/job/29104184414

I dont get how testing was working before upgrading.

@Limraj Limraj added this to the 2.8.0 milestone Aug 29, 2024
!@min-gb/dist
!@min-gb/public
!@min-gb/src
!@min-gb/*.*
Copy link
Collaborator

@Limraj Limraj Aug 29, 2024

Choose a reason for hiding this comment

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

I copied min-gb and called min-gb2 and this pattern worked as we expected: (what has already been added to the git repository does not work with gitignore)

node_modules/*
!node_modules/@min-gb
node_modules/@min-gb/vuejs-components/*
!node_modules/@min-gb/vuejs-components/dist
!node_modules/@min-gb/vuejs-components/src
!node_modules/@min-gb/vuejs-components/public
!node_modules/@min-gb/vuejs-components/*.*

but:

node_modules/*
!@min-gb/dist
!@min-gb/public
!@min-gb/src
!@min-gb/*.*

not working for dir/file that have not been added to the git repository.

You can also add this to the main gitignore file (\Scada-LTS\.gitignore):
WebContent/resources/js-ui/

then only file change from repo:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I didn't notice there was a vuejs-components subfolder

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @YuaFox,
You can also add this to the main gitignore file (\Scada-LTS.gitignore):
WebContent/resources/js-ui/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's already there
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

@YuaFox,
Okay, but I guess this pattern doesn't work, see for yourself... maybe because at the beginning there is /

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its working for me doing this command to force retracking of all files.

git rm --cached -r . && git add .

After doing this i have 0 files tracked from WebContent/resources/js-ui/ folder, but i found a problem:
.gitignore says /tomcat/lib should not be tracked but its in the repo

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @YuaFox Ok, it actually works for me, it doesn't show these changes either. (/WebContent/resources/js-ui/)
However, tracking /tomcat/lib is enabled, if it is in the sources, there is not much other option... and I don't see a pattern that could exclude this folder from tracking, there is neither the word lib nor tomcat.

YuaFox and others added 8 commits August 30, 2024 11:51
- Removed deprecated cypress functions
- Removed modern_watch_list test
- Fixed scadalts-ui .gitignore
- update lock file: scadalts-ui/package-lock.json, WebContent/resources/package-lock.json
- removed ^ form Cypress version, and update package-lock.json files;
- corrected cypress config, added: fixturesFolder, specPattern;
- added ag-grid-community lib;
- revert data_point_details.cy.js
- upgrade version Node to v22.9.0;
Copy link
Collaborator

@Limraj Limraj left a comment

Choose a reason for hiding this comment

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

There was a problem with the latest version 22.10.0 but rolling back to version 22.9.0 solved the problem. The Node version was set to a fixed version to exclude the possibility that the project would stop building for this reason.

A separate issue has also been created for e2e test fixes.

#3014

@Limraj Limraj merged commit 190960c into release/2.8.0 Oct 29, 2024
8 checks passed
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.

2 participants