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

Implemented unit test in build #30

Merged
merged 41 commits into from
Nov 3, 2023
Merged

Conversation

harmen-xb
Copy link
Contributor

@harmen-xb harmen-xb commented Oct 29, 2023

  • Refactored github workflows to only workflow files (no actions) and include build, test and lint (including test report).
  • Refactored all tsconfig files to re-use base config and all sub-folder tsconfigs to be composite.
  • Fixed error in debug launch settings so debug-mode is set correctly.
  • Upgraded jest dependencies to latest version.
  • Refactored all jest configs to have local configs which re-use generic config.
  • Performed yarn update
  • Setup wokspace jest config which references to all projects.
  • Updated gitpod config.
  • Added VSCode Dev Container config.

Removed jest dependencies from root package.json.
Moved jest-config to extentensions/crossmodel-lang  for now.
Updated workflow to pickup the jest xml.
Changed jest config to write in the root, i.s.o. out directory.
Changed scan path for publishing test results.
Updated publish test result files path.
Refactored tsconfig to all extend the base tsconfig and have specific one in the root of the workspace.
@harmen-xb harmen-xb self-assigned this Oct 29, 2023
@harmen-xb harmen-xb changed the title Feature/unit test in build Implemented unit test in build Oct 29, 2023
@harmen-xb
Copy link
Contributor Author

harmen-xb commented Oct 30, 2023

@martin-fleck-at

As part of this PR for unit tests in the build I did some extra things which I want to check with you:

  1. I first manually remove some packages from the package.json file in the root. After some other commits I realized that by doing this the yarn.lock is not updated. So I performed a 'yarn upgrade' to rebuild the yarn.lock based on the specified package versions. I do realize all dependencies are also updated if newer version are available wihin the version spec. Is this ok?
  2. At some point I got errors on the moduleResolution setting. I then looked into the Theia repository and synched the settings with that repo in this commit: 1358562. Does this have any dowside/side-effect I am not aware of?
  3. I had issues with EsLint at some point which was due to an error in the include paths, which was fixed (0c42af9). But before this fix I thought it was due to the tsconfig being wrong for the workspace or something. So I aligned it to be like the Theia or Theia Blueprint repo: ab2a8fb. Is this ok? Cause to me it looks better somehow, but again I don't know the side-effect of these changes.
  4. I enabled executing 'yarn lint' in the workflow, but only for Ubuntu (since it's the fastest). I did this because if I understand it correctly, lint should have the same result in all OSes right? (7b70a41)

@harmen-xb harmen-xb added enhancement New feature or request dependency-update labels Oct 30, 2023
Added model-server test from another branch.
@github-actions
Copy link

github-actions bot commented Oct 30, 2023

Test Results

    3 files    30 suites   1m 12s ⏱️
  68 tests   68 ✔️ 0 💤 0
207 runs  207 ✔️ 0 💤 0

Results for commit 44c07e4.

♻️ This comment has been updated with latest results.

pdated package.json in root to execute jest directly, i.s.o. via lerna per package.
Copy link
Collaborator

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thank you very much for that change, @harmen-xb! Having the tests properly execute will help us a lot down the road!

As for your questions:

  1. I'm not sure I fully follow you. Are you saying after moving some of the dependencies from the root package.json to other package.json files, the yarn.lock did not get updated properly? I would need to check up on this but I would not have expected this. Did you do a yarn or yarn install? Of course, you can do a yarn upgrade but I would be careful about it as it upgrades a lot of packages so it is better to maybe specify the particular package you want to upgrade.
  2. No, I think using Node for module resolution should be absolutely fine. I think we switched to Node16 as GLSP had that setting. But unless we need any specific features such as async imports it is definitely good idea to stay aligned with Theia.
  3. The configuration looks good to me so thank you for fixing that! I really wouldn't expect any side effects if the linting is properly applied on all our files.
  4. You are completely right, linting only does a static code analysis so there shouldn't be anything platform-specific and doing it only on Linux is definitely a good idea. I believe Theia is doing the same.

One minor thing I noticed was that when we run the linting now, we already get 3 warnings:

crossmodel-lang: [...]/crossmodel/extensions/crossmodel-lang/src/language-server/lexer/cross-model-indentation-tokens.ts
crossmodel-lang:   57:1  warning  Missing return type on function  @typescript-eslint/explicit-function-return-type
crossmodel-lang: ✖ 1 problem (0 errors, 1 warning)

@crossbreeze/model-service: [...]/crossmodel/packages/model-service/src/common/model-service-rpc.ts
@crossbreeze/model-service:   23:39  warning  'JsonRpcServer' is deprecated. since 1.39.0 use `RpcServer` instead  deprecation/deprecation
@crossbreeze/model-service: [...]/crossmodel/packages/model-service/src/node/model-service-backend-module.ts
@crossbreeze/model-service:   18:21  warning  'JsonRpcConnectionHandler' is deprecated. since 1.39.0 use `RpcConnectionHandler` instead  deprecation/deprecation
@crossbreeze/model-service: ✖ 2 problems (0 errors, 2 warnings)

Should we maybe fix them already or do you want to do this in a separate task?

Unfortunately, through the yarn upgrade we also did a Theia upgrade from 1.37.2 to 1.42.1 which broke the Electron build. I see the following issue when starting it up:

Error: Cannot find module '../src-gen/frontend/electron-main.js'
Require stack:
- [...]/crossmodel/applications/electron-app/scripts/electron-main.js
- [...]/crossmodel/node_modules/electron/dist/resources/default_app.asar/main.js

I believe this needs to be adapted to lib/frontend/electron-main.js. I didn't check for any other issues yet, not sure if you want to revert the Theia upgrade and do it separately.

@harmen-xb harmen-xb closed this Nov 3, 2023
@harmen-xb harmen-xb reopened this Nov 3, 2023
Copy link
Collaborator

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thank you for the quick update, @harmen-xb! I had another look at it and the tests run through without any issues, all linting errors and warnings are gone and everything still works as expected. Great work!

@harmen-xb harmen-xb merged commit f8c9c68 into main Nov 3, 2023
5 checks passed
@harmen-xb harmen-xb deleted the feature/unit-test-in-build branch November 3, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency-update enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants