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

generator-langium: extended yeoman generator extension to offer parsing, linking, and validation test stubs (#1282) #1298

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

sailingKieler
Copy link
Contributor

@sailingKieler sailingKieler commented Nov 20, 2023

  • providing additional automated tests for the cases of 'stubsOnly', 'vitest', and 'jest'

@dhuebner
Copy link
Contributor

@sailingKieler
Running npm run test currently fails for me with:

 FAIL  packages/generator-langium/test/yeoman-generator.test.ts > Check yeoman generator works > 3 Should produce files for Vitest
Error: EACCES: permission denied, mkdir '/__yo-gen-testing'

Could it be that there are too many ..?

Dir name in my case is: __dirname: /workspace/langium/packages/generator-langium/
Test tries to write to: Generating into directory: /__yo-gen-testing

@sailingKieler
Copy link
Contributor Author

Hi Dennis,

@sailingKieler Running npm run test currently fails for me with:

 FAIL  packages/generator-langium/test/yeoman-generator.test.ts > Check yeoman generator works > 3 Should produce files for Vitest
Error: EACCES: permission denied, mkdir '/__yo-gen-testing'

Could it be that there are too many ..?

Dir name in my case is: __dirname: /workspace/langium/packages/generator-langium/ Test tries to write to: Generating into directory: /__yo-gen-testing

The test is supposed to write __yo-gen-testing next to langium, i.e. to /workspace/__yo-gen-testing in order to not see the langium root node_modules. Did you test it on Gitpod?

@dhuebner
Copy link
Contributor

dhuebner commented Nov 21, 2023

@sailingKieler
Yes, I tested it with GitPod. I think the tests should not create files outside the workspace in any (except temp) case. However it seems that there is an additional .. in '../../../../__yo-gen-testing' that can me omitted. Can you confirm?

@sailingKieler
Copy link
Contributor Author

@dhuebner you're right, and I guess that's due to the introduction of

const __dirname = url.fileURLToPath(new URL('../', import.meta.url));
I had to rebase on. I'll fix that! Thanks a lot for testing! 🙏

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

Thanks! This is a good addition that has been missing so far.

packages/generator-langium/src/index.ts Outdated Show resolved Hide resolved
`);

// check for absensce of parser errors the classic way:
// deacivated, find a much more human readable way below!
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: "more human readable" is true for the test output in case of failure, but the readability of the test code is worse. In that regard, I prefer the straightforward one-liner.

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to keep the code as it it, but add more explanation why it's useful to optimize for test output readability.

Copy link
Contributor Author

@sailingKieler sailingKieler Nov 21, 2023

Choose a reason for hiding this comment

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

It's basically everybody's choice to switch back to the one liner version.
Regrettably there's no way of attaching custom messages in case of failures AFAIK.

I think it's a chance to show users alternatives. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some explanation:

// check for absensce of parser errors the classic way:
// deacivated, find a much more human readable way below!
// expect(document.parseResult.parserErrors).toHaveLength(0);
expect(
// here we use a (tagged) template expression to create a human readable representation
// of the AST part we are interested in and that is to be compared to our expectation;
// prior to the tagged template expression we check for validity of the parsed document object
// by means of the reusable function 'checkDocumentValid()' to sort out (critical) typos first;
checkDocumentValid(document) || s`

Copy link
Member

Choose a reason for hiding this comment

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

Regrettably there's no way of attaching custom messages in case of failures AFAIK.

There is, just use expect(value, failureMessage)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😱 Since when is that?

Copy link
Member

Choose a reason for hiding this comment

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

Since we switched to vitest :)

@sailingKieler sailingKieler force-pushed the cs/yeoman-lang-test-support branch 3 times, most recently from eda512f to 01c9028 Compare November 21, 2023 16:00
@sailingKieler
Copy link
Contributor Author

@dhuebner feel free to give another try

@sailingKieler sailingKieler force-pushed the cs/yeoman-lang-test-support branch 3 times, most recently from e12ba9c to 7b0a010 Compare November 23, 2023 14:17
@sailingKieler
Copy link
Contributor Author

okay I dropped the options, and the Jest support, apparently I'm the only lonely wolf using it (in a commercial project).

Should it then be a question, at all? Maybe not.

@sailingKieler sailingKieler force-pushed the cs/yeoman-lang-test-support branch 2 times, most recently from 6174214 to af91751 Compare November 23, 2023 15:52
@sailingKieler sailingKieler force-pushed the cs/yeoman-lang-test-support branch from af91751 to aadafcc Compare November 27, 2023 09:37
Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

Thanks, it looks really good. Just two issues:

  • I see this in the editor for tsconfig.json:
File '/workspace/langium/examples/hello-world/test/linking/linking.test.ts' is not under 'rootDir' '/workspace/langium/examples/hello-world/src'. 'rootDir' is expected to contain all source files.
  The file is in the program because:
    Matched by include pattern 'test/**/*.ts' in '/workspace/langium/examples/hello-world/tsconfig.json'
  File is ECMAScript module because '/workspace/langium/examples/hello-world/package.json' has field "type" with value "module"
  • I get the following error when I answer "Yes" to all questions:
"initialize" is not exported by "../../node_modules/vscode/extensions.js", imported by "../../node_modules/monaco-languageclient/lib/monaco-vscode-api-services.js".
file: /workspace/langium/node_modules/monaco-languageclient/lib/monaco-vscode-api-services.js:6:9
4:  * ------------------------------------------------------------------------------------------ */
5: import { ILogService, initialize, StandaloneServices } from 'vscode/services';
6: import { initialize as initializeVscodeExtensions } from 'vscode/extensions';
            ^
7: import getLanguagesServiceOverride from '@codingame/monaco-vscode-languages-service-override';
8: import getModelServiceOverride from '@codingame/monaco-vscode-model-service-override';
error during build:
RollupError: "initialize" is not exported by "../../node_modules/vscode/extensions.js", imported by "../../node_modules/monaco-languageclient/lib/monaco-vscode-api-services.js".

@kaisalmen
Copy link
Contributor

@spoenemann the second problem I will fix with a separate PR. The minor updates of monaco-vscode-api are pretty major and therefore this error surfaces here. There is CodinGame/monaco-vscode-api#291

@kaisalmen
Copy link
Contributor

⬆️ Fix is here: #1317

@sailingKieler consider upgrading to vite v5 / vitest v1 before merging this PR.

@sailingKieler
Copy link
Contributor Author

@sailingKieler consider upgrading to vite v5 / vitest v1 before merging this PR.

Wouldn't that be out of scope for this PR?

@kaisalmen
Copy link
Contributor

Wouldn't that be out of scope for this PR?

Only for the generated output, not the whole project

@sailingKieler
Copy link
Contributor Author

Only for the generated output, not the whole project

Of course 🤦🏽‍♂️ 😅

@sailingKieler sailingKieler force-pushed the cs/yeoman-lang-test-support branch from aadafcc to 89e458a Compare December 12, 2023 13:11
@sailingKieler sailingKieler force-pushed the cs/yeoman-lang-test-support branch from 89e458a to 589aef4 Compare December 12, 2023 13:18
@sailingKieler
Copy link
Contributor Author

consider upgrading to vite v5 / vitest v1 before merging this PR.

Tested upgrading to vitest ~1.0.0 but that requires Node.js v18, see
https://github.com/vitest-dev/vitest/blob/66933c3a590fa5cff35e252a5f107b0f41d9248b/packages/vitest/package.json#L103-L105 and
https://github.com/vitest-dev/vitest/blob/66933c3a590fa5cff35e252a5f107b0f41d9248b/packages/vitest/package.json#L111-L113

The latter causes a conflict during npm i:

npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR! 
npm ERR! While resolving: [email protected]
npm ERR! Found: @types/[email protected]
npm ERR! node_modules/@types/node
npm ERR!   dev @types/node@"~16.18.41" from the root project
npm ERR!   peerOptional @types/node@">= 14" from [email protected]
npm ERR!   node_modules/vite
npm ERR!     dev vite@"~4.4.11" from the root project
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peerOptional @types/node@"^18.0.0 || >=20.0.0" from [email protected]
npm ERR! node_modules/vitest
npm ERR!   dev vitest@"~1.0.0" from the root project
npm ERR! 
npm ERR! Conflicting peer dependency: @types/[email protected]
npm ERR! node_modules/@types/node
npm ERR!   peerOptional @types/node@"^18.0.0 || >=20.0.0" from [email protected]
npm ERR!   node_modules/vitest
npm ERR!     dev vitest@"~1.0.0" from the root project

Do you wanna switch to v18 as minimally required node.js version?

@sailingKieler sailingKieler force-pushed the cs/yeoman-lang-test-support branch from 589aef4 to b03513b Compare December 12, 2023 13:36
@kaisalmen
Copy link
Contributor

Do you wanna switch to v18 as minimally required node.js version?

The question is why not. node 18 was the last LTS version. 20 is the current LTS version. 16 is outdated.

@sailingKieler
Copy link
Contributor Author

Do you wanna switch to v18 as minimally required node.js version?

The question is why not. node 18 was the last LTS version. 20 is the current LTS version. 16 is outdated.

Filed dedicated PR #1319

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

@sailingKieler sailingKieler force-pushed the cs/yeoman-lang-test-support branch 5 times, most recently from ea60a27 to 21e16c4 Compare December 20, 2023 13:51
@sailingKieler
Copy link
Contributor Author

@msujew @kaisalmen I upgraded the versions of node.js & vitest in the generated package.json, and fixed some further tiny bugs.

Can you guys have a final look?

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just minor typos, see below 👍

…ng, linking, and validation test stubs (#1282)

* providing an additional automated test running 'npm test' and checking its proper termination
@sailingKieler sailingKieler force-pushed the cs/yeoman-lang-test-support branch from 21e16c4 to 4f28473 Compare December 21, 2023 15:10
@sailingKieler sailingKieler merged commit 2ca2b3a into main Dec 21, 2023
5 checks passed
@sailingKieler sailingKieler deleted the cs/yeoman-lang-test-support branch December 21, 2023 15:12
@spoenemann spoenemann added this to the v3.0.0 milestone Jan 9, 2024
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.

6 participants