Skip to content

Conversation

@skrustev
Copy link
Member

@skrustev skrustev commented Jun 4, 2025

Related to #15616

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@skrustev skrustev requested a review from damyanpetev June 4, 2025 07:57
@skrustev skrustev changed the title tests(prograssBar): Fix tests for Circular and Linear Bars not clearing test bed. tests(progressBar): Fix tests for Circular and Linear Bars not clearing test bed. Jun 4, 2025
Comment on lines +30 to +33
// Remove elements manually from DOM, because CircularBar overrides the id the TestBed uses
fixture.elementRef.nativeElement.remove();
fixture.destroy();
});
Copy link
Member

Choose a reason for hiding this comment

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

This is few lines above in the beforeEach for the exact same reason:

// For test fixture destroy
progress.id = "root1";

Did that stop working? Because it's used in other component's tests too.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's a follow up to the PR adding it. Should it be removed? Did it never work?

Copy link
Contributor

Choose a reason for hiding this comment

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

@skrustev @damyanpetev same question as in #15663. Should we close them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I guess it has been added after I committed the changes. It appears to be working, apparently by setting id with name root in it. Will close this PR

@ChronosSF ChronosSF changed the base branch from master to 20.0.x June 9, 2025 11:58
@skrustev skrustev closed this Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants