Allow instrumentation to be turned on using .ember-cli by patocallaghan · Pull Request #7491 · ember-cli/ember-cli

patocallaghan · 2017-12-06T21:13:08Z

Addresses #7429

This is a WIP so hoping to get some feedback. I'm open to changing the namings + approach if required. What I initially thought to be straightforward has turned into somewhat of a rabbit-hole 😄

abstracted retrieving the Yam config for the project out to utilities/get-config.
Introducing the config into utiltities/instrumentation meant needing to move some things around as there are circular dependencies between Instrumentation => Project => Instrumentation. Otherwise it would blow up on init.

Testing is pretty tricky and not fully 🍏 yet. Due to:

Finding an issue with yam whereby a falsey value returns undefined. I have a fix here [FIX] Fix issue where falsey property is returned as undefined twokul/yam#16

The _enableFSMonitorIfInstrumentationEnabled tests in models/instrumentation-test.js are not atomic. Using heimdall-fs-monitor to wrap around fs.statSync doesn't clean up after itself so affects later tests.

ember-cli/tests/unit/models/instrumentation-test.js

Lines 36 to 83 in d9c0159

    
           describe('._enableFSMonitorIfInstrumentationEnabled', function() { 
        
             let originalStatSync = fs.statSync; 
        
             beforeEach(function() { 
        
               expect(!!process.env.BROCCOLI_VIZ).to.eql(false); 
        
               expect(!!process.env.EMBER_CLI_INSTRUMENTATION).to.eql(false); 
        
             }); 
        
             afterEach(function() { 
        
               td.reset(); 
        
             }); 
        
             it('if VIZ is NOT enabled, do not monitor', function() { 
        
               let monitor = Instrumentation._enableFSMonitorIfInstrumentationEnabled(); 
        
               try { 
        
                 expect(fs.statSync).to.equal(originalStatSync); 
        
                 expect(monitor).to.eql(undefined); 
        
               } finally { 
        
                 if (monitor) { 
        
                   monitor.stop(); 
        
                 } 
        
               } 
        
             }); 
        
             it('if VIZ is enabled, monitor', function() { 
        
               process.env.BROCCOLI_VIZ = '1'; 
        
               let monitor = Instrumentation._enableFSMonitorIfInstrumentationEnabled(); 
        
               try { 
        
                 expect(fs.statSync).to.not.equal(originalStatSync); 
        
               } finally { 
        
                 if (monitor) { 
        
                   monitor.stop(); 
        
                 } 
        
               } 
        
             }); 
        
             it('if instrumentation is enabled, monitor', function() { 
        
               process.env.EMBER_CLI_INSTRUMENTATION = '1'; 
        
               let monitor = Instrumentation._enableFSMonitorIfInstrumentationEnabled(); 
        
               try { 
        
                 expect(fs.statSync, 'fs.statSync').to.not.equal(originalStatSync, '[original] fs.statSync'); 
        
               } finally { 
        
                 if (monitor) { 
        
                   monitor.stop(); 
        
                 } 
        
               } 
        
             }); 
        
           });

. In particular it's because 1) we don't reassign fs.statSync back to it's initial state after each test and 2) calling FSMonitor() sets some module-level vars in heimdall-fs-monitor (isFirstInstance, isMonitorRegistrant) which means trying to set up FSMonitor() in a following test doesn't fully work. It doesn't re-"attach" the monitor functions.

Any feedback on the approach in general is appreciated. Also any ideas how to workaround around the heimdall-fs-monitor testing issue. I could submit a PR to fix that repo too if useful?

patocallaghan · 2017-12-06T21:13:57Z

lib/models/instrumentation.js

 let instrumentationEnabled = utilsInstrumentation.instrumentationEnabled;

-function _enableFSMonitorIfInstrumentationEnabled() {
+function _enableFSMonitorIfInstrumentationEnabled(config) {


Need to be able to pass the config in here so we can "mock" it in tests? Is this a smell or okay to do?

This looks fine to me.

patocallaghan · 2017-12-06T21:15:10Z

lib/models/project.js

    return cli.instrumentation;
  }

+  let Instrumentation = require('./instrumentation');


I needed to move the Instrumentation require inline otherwise it blew up on init due to circular dependencies between Instrumentation => getConfig => Project => Instrumentation and things not being fully set up.

Can we add a comment that describes this?

Also note the only reason we need the project in get-config is to get the project root, so it's more immediately clear to a future traveler what they would need to tackle if they wanted to change things.

patocallaghan · 2017-12-06T21:16:37Z

lib/utilities/get-config.js

+  primary: Project.getProjectRoot(),
+});
+
+module.exports = function getConfig(mockedConfig) {


allow passing in a mocked config to support current mocking behavior in some tests

Super minor, but within here can we call it configOverride instead of mockedConfig?

patocallaghan · 2017-12-06T21:18:39Z

tests/unit/models/instrumentation-test.js

    beforeEach(function() {
      expect(!!process.env.BROCCOLI_VIZ).to.eql(false);
      expect(!!process.env.EMBER_CLI_INSTRUMENTATION).to.eql(false);
+      expect(fs.statSync).to.equal(originalStatSync);


Assert that we always start with clean slate on fs.statSync

patocallaghan · 2017-12-06T21:18:58Z

tests/unit/models/instrumentation-test.js

    });

    afterEach(function() {
+      fs.statSync = originalStatSync;


Reset fs.statSync to its original state

Let's not do this; we should instead fix heimdall-fs-monitor

patocallaghan · 2017-12-06T21:21:23Z

tests/unit/models/instrumentation-test.js

      process.env.EMBER_CLI_INSTRUMENTATION = '1';
      let monitor = Instrumentation._enableFSMonitorIfInstrumentationEnabled();
+      try {
+        expect(fs.statSync).to.not.equal(originalStatSync);


This test fails due to heimdall-fs-monitor not fully cleaning up after itself. Some module level variables are set in a previous test which cause it be isEnabled already. This means when calling FSMonitor() a second time, it doesn't re-"attach" the monitor functions around the fs functions.

okay it looks like this problem in heimdall-fs-monitor was actually fixed a while ago. I've released 0.2.0 to deal with stop not cleaning up properly.

hjdivad

Couple of very minor suggestions but otherwise LGTM (sans greening up tests of course)

hjdivad · 2017-12-12T16:46:24Z

lib/models/instrumentation.js

 let instrumentationEnabled = utilsInstrumentation.instrumentationEnabled;

-function _enableFSMonitorIfInstrumentationEnabled() {
+function _enableFSMonitorIfInstrumentationEnabled(config) {


This looks fine to me.

hjdivad · 2017-12-12T16:48:57Z

lib/models/project.js

    return cli.instrumentation;
  }

+  let Instrumentation = require('./instrumentation');


Can we add a comment that describes this?

Also note the only reason we need the project in get-config is to get the project root, so it's more immediately clear to a future traveler what they would need to tackle if they wanted to change things.

hjdivad · 2017-12-12T17:00:28Z

lib/utilities/get-config.js

+  primary: Project.getProjectRoot(),
+});
+
+module.exports = function getConfig(mockedConfig) {


Super minor, but within here can we call it configOverride instead of mockedConfig?

hjdivad · 2017-12-12T17:24:28Z

tests/unit/models/instrumentation-test.js

    });

    afterEach(function() {
+      fs.statSync = originalStatSync;


Let's not do this; we should instead fix heimdall-fs-monitor

hjdivad · 2017-12-12T18:05:00Z

tests/unit/models/instrumentation-test.js

      process.env.EMBER_CLI_INSTRUMENTATION = '1';
      let monitor = Instrumentation._enableFSMonitorIfInstrumentationEnabled();
+      try {
+        expect(fs.statSync).to.not.equal(originalStatSync);


okay it looks like this problem in heimdall-fs-monitor was actually fixed a while ago. I've released 0.2.0 to deal with stop not cleaning up properly.

hjdivad · 2017-12-12T18:10:10Z

@patocallaghan updated heimdall-fs-monitor on your branch; stop should clean up better now, LMK if you're still seeing issues here.

patocallaghan · 2017-12-13T10:55:47Z

Appreciate the feedback @hjdivad. Will fix up per your comments.

patocallaghan · 2017-12-18T00:17:19Z

tests/unit/settings-file/leek-options-test.js


  before(function() {
-    settings = new Yam('ember-cli', {
+    let mockedYam = new Yam('ember-cli', {


Needed to slightly refactor this test to get it to pass.

patocallaghan · 2017-12-18T00:20:58Z

lib/utilities/get-config.js

+  if (configOverride) {
+    return configOverride;
+  }
+  return new Yam('ember-cli', {


I needed to bring the new Yam() into the getConfig function because it was failing the tests/acceptance/pods-destroy-test.js tests otherwise. Preferably the new Yam would happen at the module scope but in the pod-destroy tests the config wasn't being re-calculated after modifying the .ember-cli file.

patocallaghan · 2017-12-18T00:25:31Z

@hjdivad made some changes per your feedback. Also tests should be 🍏 now.

patocallaghan · 2018-01-08T13:46:23Z

@hjdivad I just did rebase against current master and the branch is still 🍏

twokul

Couple of minor things but I think this looks good apart from @hjdivad comments

twokul · 2018-01-08T18:44:48Z

lib/models/project.js

    return cli.instrumentation;
  }

+  // Instrumentation `require` needs to occur inline due to circular dependencies between Instrumentation => getConfig => Project => Instrumentation. getConfig is used in Project to get the project root.


sorry for nitpicking but could you wrap break this comment into separate lines, around 80 characters long?

twokul · 2018-01-08T18:48:49Z

lib/utilities/instrumentation.js


-function instrumentationEnabled() {
-  return vizEnabled() || process.env.EMBER_CLI_INSTRUMENTATION === '1';
+function isConfigEnabled(configOverride) {


maybe better to name it isInstrumetationConfigEnabled?

patocallaghan · 2018-01-09T00:20:36Z

No problem @twokul, I've addressed yours and @hjdivad's comments. I've also resolved some merge conflicts with the yarn.lock on master.

stefanpenner · 2018-01-10T01:02:01Z

lib/utilities/get-config.js

+  if (configOverride) {
+    return configOverride;
+  }
+  return new Yam('ember-cli', {


should we load a new Yam on each invocation of getConfig, or memoize?

@stefanpenner I'd initially tried have the new Yam line at the module level but had to refactor because tests which relied on mocking the config failed (pod tests I believe). I can't remember the specific reasoning but I'll have another go and see.

hjdivad

LGTM although it'd be good to look into @stefanpenner's comment re Yam instances

hjdivad · 2018-01-10T18:14:22Z

thanks @patocallaghan!

patocallaghan · 2018-01-10T22:18:51Z

@stefanpenner @hjdivad I've just pushed a little spike of memoizing the Yam config in getConfig by moving it back to the module level. By doing that it fails a bunch of pod tests which rely on having the .ember-cli file having usePods: true. For example this test here:

ember-cli/tests/acceptance/pods-destroy-test.js

Lines 133 to 142 in 6fe4412

    
           it('.ember-cli usePods setting correctly destroys component', function() { 
        
             let commandArgs = ['component', 'x-foo']; 
        
             let files = [ 
        
               'app/components/x-foo/component.js', 
        
               'app/components/x-foo/template.hbs', 
        
               'tests/integration/components/x-foo/component-test.js', 
        
             ]; 
        
             return assertDestroyAfterGenerateWithUsePods(commandArgs, files); 
        
           });

When the test is initially run the get-config module is loaded and the config is generated but then when we get to the assertDestroyAfterGenerateWithUsePods we augment the config with the usePods: true setting:

ember-cli/tests/acceptance/pods-destroy-test.js

Line 94 in 6fe4412

    
           replaceFile('.ember-cli', '"disableAnalytics": false', '"disableAnalytics": false,\n"usePods" : true\n');

But as we've memoized the config on module load we don't have a Yam with usePods set.

I've added some sort of solution but to be honest it feels really wrong. When in test mode it generates a new Yam but when not it uses the memoized one. There's a precedent for this elsewhere in the tests but it still doesn't make it right 😄

If anyone has any suggestions for a better approach I'm all ears :)

patocallaghan · 2018-01-17T21:58:19Z

@stefanpenner @hjdivad @twokul any thoughts re my last comment here? Happy to rethink the testing if it's not acceptable.

FYI I rebased against master and now the branch is failing. It appears master is also failing with the same error.

hjdivad · 2018-01-17T22:30:33Z

lib/utilities/get-config.js

+
+let config = generateConfig();
+
+module.exports = function getConfig(configOverride, shouldRegenerateConfig) {


instead of adding another parameter for this, what do you think about exporting a separate reset function for tests?

in es6 this would be a non default export, but in this case it should be fine to hang it off the default export. eg

let config; module.exports = function getConfig(configOverride) { if (configOverride) { return configOverride; } if (config === undefined) { config = generateConfig(); } return config; } // private; used in testing module.exports._reset = function() { config = undefined; }

I'll give that a shot. Thanks 👍

@hjdivad it turns out the problematic tests were actually removed in a recent commit 😄 So there's no longer a need to jump through hoops to get the suite to pass. Tests are 🍏 locally and the config memoization works.

oh nice; so if you rebase and push we should expect the PR to be green then

it looks like master is still 🔴

I think these tests we just moved into ember-source so that is why we're seeing the failures over there now when upgrading ember-cli.

@patocallaghan if it suits, let's have a chat about this tomorrow

stefanpenner · 2018-01-18T16:59:50Z

is there a downside to not memoizing?

patocallaghan · 2018-01-18T22:53:01Z

@stefanpenner I guess the downside would be that we're creating new Yam objects every time we call getConfig even though it doesn't change over the lifecycle of the command (or can it?)

patocallaghan · 2018-01-29T17:18:20Z

@hjdivad and @stefanpenner I've rebased again and the PR is now 🍏

hjdivad · 2018-01-31T20:21:28Z

thanks @patocallaghan!

GavinJoyce · 2018-04-04T18:59:00Z

This appears to break some generators which use "usePods": true in .ember-cli: https://github.com/emberjs/ember.js/pull/16465/files#r179132445

patocallaghan commented Dec 6, 2017

View reviewed changes

patocallaghan requested review from hjdivad, rwjblue and twokul December 11, 2017 16:00

hjdivad suggested changes Dec 12, 2017

View reviewed changes

patocallaghan force-pushed the patoc/config-instrumentation branch from 33141d2 to f88c4a2 Compare December 18, 2017 00:14

patocallaghan commented Dec 18, 2017

View reviewed changes

patocallaghan changed the title ~~[WIP] Allow instrumentation to be turned on using .ember-cli~~ Allow instrumentation to be turned on using .ember-cli Dec 18, 2017

patocallaghan force-pushed the patoc/config-instrumentation branch from f88c4a2 to 0e8853e Compare January 8, 2018 12:42

twokul approved these changes Jan 8, 2018

View reviewed changes

patocallaghan force-pushed the patoc/config-instrumentation branch from 0e8853e to 4f76e86 Compare January 9, 2018 00:05

stefanpenner reviewed Jan 10, 2018

View reviewed changes

hjdivad approved these changes Jan 10, 2018

View reviewed changes

patocallaghan force-pushed the patoc/config-instrumentation branch from ab1d50c to fa901c2 Compare January 17, 2018 21:15

hjdivad reviewed Jan 17, 2018

View reviewed changes

patocallaghan force-pushed the patoc/config-instrumentation branch from fa901c2 to 5bed858 Compare January 17, 2018 23:27

patocallaghan force-pushed the patoc/config-instrumentation branch from 5bed858 to d284dc8 Compare January 18, 2018 21:52

patocallaghan and others added 2 commits January 29, 2018 14:33

Allow instrumentation to be turned on using .ember-cli

5d5801a

Update heimdalljs-fs-monitor to 0.2.0

ca48df5

patocallaghan force-pushed the patoc/config-instrumentation branch from d284dc8 to ca48df5 Compare January 29, 2018 14:34

hjdivad merged commit f46648f into ember-cli:master Jan 31, 2018

rwjblue mentioned this pull request Apr 4, 2018

Update ember-cli and fix failing tests emberjs/ember.js#16465

Merged

This was referenced Apr 5, 2018

remove config caching #7739

Merged

revert "Allow instrumentation to be turned on using .ember-cl" #7738

Closed

patocallaghan mentioned this pull request Apr 12, 2018

Turning on Instrumentation using ember-cli-dotenv #7429

Closed

	describe('._enableFSMonitorIfInstrumentationEnabled', function() {
	let originalStatSync = fs.statSync;

	beforeEach(function() {
	expect(!!process.env.BROCCOLI_VIZ).to.eql(false);
	expect(!!process.env.EMBER_CLI_INSTRUMENTATION).to.eql(false);
	});

	afterEach(function() {
	td.reset();
	});

	it('if VIZ is NOT enabled, do not monitor', function() {
	let monitor = Instrumentation._enableFSMonitorIfInstrumentationEnabled();
	try {
	expect(fs.statSync).to.equal(originalStatSync);
	expect(monitor).to.eql(undefined);
	} finally {
	if (monitor) {
	monitor.stop();
	}
	}
	});

	it('if VIZ is enabled, monitor', function() {
	process.env.BROCCOLI_VIZ = '1';
	let monitor = Instrumentation._enableFSMonitorIfInstrumentationEnabled();
	try {
	expect(fs.statSync).to.not.equal(originalStatSync);
	} finally {
	if (monitor) {
	monitor.stop();
	}
	}
	});

	it('if instrumentation is enabled, monitor', function() {
	process.env.EMBER_CLI_INSTRUMENTATION = '1';
	let monitor = Instrumentation._enableFSMonitorIfInstrumentationEnabled();
	try {
	expect(fs.statSync, 'fs.statSync').to.not.equal(originalStatSync, '[original] fs.statSync');
	} finally {
	if (monitor) {
	monitor.stop();
	}
	}
	});
	});


		let config = generateConfig();

		module.exports = function getConfig(configOverride, shouldRegenerateConfig) {

Conversation

patocallaghan commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patocallaghan Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patocallaghan Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hjdivad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hjdivad commented Dec 12, 2017

Uh oh!

patocallaghan commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patocallaghan commented Dec 18, 2017

Uh oh!

patocallaghan commented Jan 8, 2018

Uh oh!

twokul left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patocallaghan commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hjdivad left a comment

Choose a reason for hiding this comment

Uh oh!

hjdivad commented Jan 10, 2018

Uh oh!

patocallaghan commented Jan 10, 2018

Uh oh!

patocallaghan commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patocallaghan commented Dec 6, 2017 •

edited

Loading

patocallaghan Dec 6, 2017 •

edited

Loading

patocallaghan Dec 6, 2017 •

edited

Loading

twokul left a comment •

edited

Loading

GavinJoyce commented Apr 4, 2018 •

edited

Loading