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

WaferMap test expectations are not executing #1746

Closed
rajsite opened this issue Jan 10, 2024 · 1 comment · Fixed by #1781
Closed

WaferMap test expectations are not executing #1746

rajsite opened this issue Jan 10, 2024 · 1 comment · Fixed by #1781
Assignees

Comments

@rajsite
Copy link
Member

rajsite commented Jan 10, 2024

🧹 Tech Debt

Several wafer map tests are resulting in warns on nimble builds such as the following: https://github.com/ni/nimble/actions/runs/7475603903/job/20344086522#step:17:262

'Spec 'Wafermap Prerendering module with undefined values should have empty color fill style' has no expectations.'

The warning states that no expect statements are running for the test. However the source of the test shows expect statements:

it('should have empty color fill style', () => {
for (const dieRenderInfo of prerenderingModule.diesRenderInfo) {
expect(dieRenderInfo.fillStyle).toEqual(emptyDieColor);
}
});

Seems like the tests are incorrectly written / not validating behaviors as expected.

The pattern that likely let this issue through is that the tests have conditionally executing expect statements which is an issue discussed here: ni/javascript-styleguide#125 (comment)

To address this tech debt the wafer map tests should be hardened to avoid conditionally executing expect statements as discussed in the above style guide issue.

@rajsite rajsite added tech debt triage New issue that needs to be reviewed labels Jan 10, 2024
rajsite added a commit that referenced this issue Jan 16, 2024
…e clean-up (#1748)

# Pull Request

## 🤨 Rationale

Fixes a couple of small issues noticed recently:

- Tests were printing out console warnings
- Some component callback lifecycles were not calling their super
implementation
- When test failures happened the current spec would stop making it
difficult to see all the failures and handle them

## 👩‍💻 Implementation

- Elevated console warn and console errors to test failures
- Updated lifecycle callbacks to call super (and renamed some methods
that weren't actually lifecycle callbacks, which was my idea, I was
wrong 😅). Created a Manual Regression Validation issue to hopefully
track that a little better in the future:
#1747
- Updated the jasmine configuration to continue running a test suite
even on failure. We can see if it is useful and easily switch back if it
is annoying. Was useful locally.
- Filed an issue for some Wafer Map tests that are running incorrectly:
#1746

## 🧪 Testing

Rely on CI.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@m-akinc m-akinc added triage New issue that needs to be reviewed and removed triage New issue that needs to be reviewed labels Jan 23, 2024
@m-akinc
Copy link
Contributor

m-akinc commented Jan 23, 2024

@munteannatan Just a heads up.

@m-akinc m-akinc removed the triage New issue that needs to be reviewed label Jan 30, 2024
rajsite pushed a commit that referenced this issue Feb 7, 2024
# Pull Request

## 🤨 Rationale

Fix for the issue regarding warnings in tests for missing expects 
Fixes #1746

## 👩‍💻 Implementation

changed testing approach to match arrays

## 🧪 Testing

ci test pass

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@m-akinc m-akinc moved this from In progress to Done in Nimble Design System Priorities Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

3 participants