-
Notifications
You must be signed in to change notification settings - Fork 34
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
test: library integration tests #1725
base: main
Are you sure you want to change the base?
Conversation
f248a91
to
53c3a0a
Compare
7bea4b5
to
5bcbf47
Compare
5bcbf47
to
ab8a8f6
Compare
bfce00d
to
8f216a6
Compare
f315253
to
d77732a
Compare
c4bde64
to
3efc10c
Compare
0d5a0c0
to
87bec2b
Compare
e02ef2b
to
59eb752
Compare
test('should add Otter Application to existing Angular app', () => { | ||
const { workspacePath, isInWorkspace, appName, o3rVersion } = o3rEnvironment.testEnvironment; | ||
const execAppOptions = {...getDefaultExecSyncOptions(), cwd: workspacePath}; | ||
packageManagerExec({script: 'ng', args: ['add', `@o3r/core@${o3rVersion}`, '--preset', 'all', '--project-name', appName, '--skip-confirmation']}, execAppOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we test ng add @o3r/application
instead ?
@@ -139,7 +139,7 @@ export function updateLocalization(options: { projectName?: string | null | unde | |||
workspaceProject.architect.build.options?.assets?.map((a: { glob: string; input: string; output: string }) => a.output).find((output: string) => output === '/localizations'); | |||
|
|||
if (!alreadyExistingBuildOption) { | |||
workspaceProject.architect.build.options.assets.push({ | |||
(workspaceProject.architect.build.options.assets || []).push({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be less confusing to just add the condition in the if
instead of creating a useless array
if (!alreadyExistingBuildOption && workspaceProject.architect.build.options.assets) {
untouchedApp, | ||
untouchedAppPath, | ||
untouchedLib, | ||
untouchedLibPath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I can tell we'll never have to make different check between untouchedApp
and untouchedLib
. We always want to check that they are untouched no matter the test
So I would suggest to simplify the tests to only export an array of paths to be checked as untouched
[untouchedAppPath, untouchedLibPath]
we could even consider writing a custom matcher to make it more readable
expect(gitDiff).not.toHaveTouched(path)
or something
59eb752
to
b75364f
Compare
const execAppOptions = {...getDefaultExecSyncOptions(), cwd: workspacePath}; | ||
packageManagerExec({script: 'ng', args: ['add', `@o3r/apis-manager@${o3rVersion}`, '--skip-confirmation', '--project-name', projectName]}, execAppOptions); | ||
const packageManager = getPackageManager(); | ||
const isYarnTest = packageManager.startsWith('yarn'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this 'isYarnTest' constant be exported by o3rEnvironment.testEnvironment
?
Proposed change
Enhance the integration test campaign with tests for libraries.
@o3r/create
should generate a project with a library@o3r/workspace
schematic for app and lib. Currently in create int tests, to be moved in workspace ones. feat(sdk): generate an sdk with spec from npm #1678 should be merged first.Ensure test coverage for libs (similar to what we have for apps) and add assertions for app tests for: