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

Tests - Package & Create Targets #73

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

AdrianRomanski
Copy link
Member

No description provided.

@AdrianRomanski AdrianRomanski changed the title Test: Package & Create Targets Tests - Package & Create Targets Jan 8, 2025
@AdrianRomanski AdrianRomanski marked this pull request as ready for review January 10, 2025 09:03
@AdrianRomanski
Copy link
Member Author

AdrianRomanski commented Jan 10, 2025

This pull request bring the targets to 100% code coverage :)
Together with #69

Zrzut ekranu 2025-01-10 o 10 25 39

Comment on lines 50 to 53
* When you pass your own namedInputs (like you would in a project.json file) via the inferred tasks plugin,
* the tasks pipeline ignores them and throws this error.
* Some Nx plugins use the default namedInput, probably for that reason,
* but I'm concerned that if developers change those inputs, it might lead to undesired behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* When you pass your own namedInputs (like you would in a project.json file) via the inferred tasks plugin,
* the tasks pipeline ignores them and throws this error.
* Some Nx plugins use the default namedInput, probably for that reason,
* but I'm concerned that if developers change those inputs, it might lead to undesired behavior.
* When you pass your own `namedInputs` (like you would in a `project.json` file) via the inferred tasks plugin,
* the tasks pipeline ignores them and throws this error.
* Some Nx plugins use the default `namedInput`, probably for that reason,
* but I'm concerned that if developers change those inputs, it might lead to undesired behaviour.

I assume this situation changed since the discussion in #62 made progress.

Copy link
Member Author

@AdrianRomanski AdrianRomanski Jan 18, 2025

Choose a reason for hiding this comment

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

I have pushed the changes you suggested. Should i leave it in code if the situation changed?

Copy link
Contributor

@BioPhoton BioPhoton left a comment

Choose a reason for hiding this comment

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

Another nice testing PR!! <3

Very nice you even checked the coverage 💪! I found slight things we could adjust... have a look.

@BioPhoton
Copy link
Contributor

Any updates?

@AdrianRomanski
Copy link
Member Author

I will push The changes today/tommorow

@AdrianRomanski
Copy link
Member Author

AdrianRomanski commented Jan 18, 2025

Things done after review

  1. Rephrasing docs, formatting
  2. Change negations !boolean to is false
  3. Change boolean to is true for consistency
  4. Change not empty to given and empty to not given
  5. Change configuration with correct structure to a config
  6. Delete correct structure from other tests for consistency
  7. Remove implementation details from docs
  8. DependsOn checks - getPkgTargets()

Self Review

  1. Delete duplicate test
  2. Grouping tests by category and happy/unhappy path
  3. Remove every const that were used once (Learned from Tests - Environment Targets #69)
  4. Make generation tests check only one object at time or either whole structure
  5. Delete correct structure from getPkgTargets() test names (consistency with create-targets)

Comment on lines +229 to +231
expect(nxDevkitMockModule.logger.warn).toHaveBeenCalledTimes(0);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(nxDevkitMockModule.logger.warn).toHaveBeenCalledTimes(0);
});
expect(nxDevkitMockModule.logger.warn).not.toHaveBeenCalled();
});

Comment on lines +241 to +242
expect(nxDevkitMockModule.logger.warn).toHaveBeenCalledTimes(0);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(nxDevkitMockModule.logger.warn).toHaveBeenCalledTimes(0);
});
expect(nxDevkitMockModule.logger.warn).not.toHaveBeenCalled();
});

Comment on lines +60 to +76
let normalizeCreateNodesOptionsSpy: MockInstance;
let isEnvProjectSpy: MockInstance;
let isPkgSpy: MockInstance;
let verdaccioTargetsSpy: MockInstance;
let getEnvTargetsSpy: MockInstance;
let updateEnvTargetNamesSpy: MockInstance;

beforeEach((): void => {
normalizeCreateNodesOptionsSpy = vi
.spyOn(normalizeCreateNodesSpyModule, 'normalizeCreateNodesOptions')
.mockReturnValue(normalizedOptions);
isEnvProjectSpy = vi
.spyOn(environmentTargetsModule, 'isEnvProject')
.mockReturnValue(true);
isPkgSpy = vi
.spyOn(packageTargetsSpyModule, 'isPkgProject')
.mockReturnValue(true);
Copy link
Contributor

@BioPhoton BioPhoton Feb 2, 2025

Choose a reason for hiding this comment

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

General best practices for setup spy's and configuring spy's:

  • setup in describe
  • configuring with potential defaults in beforeEach
  • actual mock return in the it block

Copy link
Contributor

@BioPhoton BioPhoton left a comment

Choose a reason for hiding this comment

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

Thanks for the tests!
I left some comments

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