fix(rum-react): respect active flag in react integration #392
Conversation
vigneshshanmugam
commented
Aug 26, 2019
•
vigneshshanmugam
commented
- fixes framework integrations should respect active flag #386
- blocks the kibana integration with RUM.
Codecov Report
@@ Coverage Diff @@
## master #392 +/- ##
=========================================
- Coverage 94.34% 94.24% -0.1%
=========================================
Files 40 39 -1
Lines 1804 1791 -13
Branches 369 366 -3
=========================================
- Hits 1702 1688 -14
- Misses 97 100 +3
+ Partials 5 3 -2
|
| Component | ||
| ) | ||
| expect(WrappedComponent).toEqual(Component) | ||
| expect(transactionService.startTransaction).not.toHaveBeenCalled() |
There was a problem hiding this comment.
transactionService.startTransaction won't be called unless the component is rendered, so this probably always passes regardless of the test!
There was a problem hiding this comment.
I added the component to get rendered as well in previous line for double checking.
if you comment out the config.isActive check in the source, this test would fail which is expected.
There was a problem hiding this comment.
Ok, I thought maybe you intended to test with the component that you created in the test instead instead of the one created in TestComponent. Might be a bit better to group the code together so it's clear which checks are intended for those parts, but it's not very important.
| it('should work if apm is disabled or not initialized', function() { | ||
| TestComponent(new ApmBase(createServiceFactory(), true)) | ||
| TestComponent(new ApmBase(createServiceFactory(), false)) | ||
| TestComponent(new ApmBase(serviceFactory, true)) |
There was a problem hiding this comment.
This might be not related to this PR, but I'm not sure why this test passes. We probably should investigate why when we pass disable = true to ApmBase, it still calls startTransaction.
Do you happen to know?
Either way we can address that separately if you prefer.
There was a problem hiding this comment.
I guess it does not matter, component should get rendered if APM is enabled/disabled and we don't check if startTransaction is called or not. So I am not sure what is the issue here?
There was a problem hiding this comment.
We probably can improve this test a bit, let's do it separately!
There was a problem hiding this comment.
@vigneshshanmugam LGTM, let's merge once the tests are green!
* fix(rum-react): respect active flag in react integration * remove logging when inactive * add check for same component * use apm.init instead of config init
* fix(rum-react): respect active flag in react integration * remove logging when inactive * add check for same component * use apm.init instead of config init
