-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(trace-view): Removing trace-view-v1 flag #94474
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #94474 +/- ##
==========================================
+ Coverage 87.90% 87.99% +0.08%
==========================================
Files 10441 10400 -41
Lines 603609 602710 -899
Branches 23491 23312 -179
==========================================
- Hits 530574 530325 -249
+ Misses 72669 71915 -754
- Partials 366 470 +104 |
const organization = OrganizationFixture({ | ||
features: ['trace-view-v1'], // only testing against new trace view | ||
}); | ||
const organization = OrganizationFixture({}); |
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.
const organization = OrganizationFixture({}); | |
const organization = OrganizationFixture(); |
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.
Lots of files but individual changes look ok to me. In the future, would be great if this could be split up a bit more (for example const organization = OrganizationFixture({});
changes could be its own pr), or if we had made prepared branching test changes incrementally as the trace view was being developed.
@@ -209,7 +209,7 @@ describe('Breadcrumbs', () => { | |||
|
|||
expect(screen.getByText('/settings/')).toHaveAttribute( | |||
'href', | |||
'/organizations/org-slug/insights/backend/project-slug:abcdabcdabcdabcdabcdabcdabcdabcd/?referrer=breadcrumbs' | |||
'/organizations/org-slug/traces/trace/undefined/?referrer=breadcrumbs&statsPeriod=14d' |
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.
Any way we can add a mock somewhere so that the trace id isn't undefined
? Feels like that should be part of the test. Unless this flow isn't valid anymore, in which case maybe it makes sense to just delete this test
Acceptance tests are failing, but we spoke about this on slack. Seems like these are for the old event details page which is deprecated and should be removed. |
… fixing broken one
This PR removes all usage of the 'trace-view-v1' flag from the FE. It has been in full GA over the last couple quarters and we want the linked functionality to be enabled by default in self hosted.