embermap / ember-cli-fastboot-testing

Test your FastBoot-rendered HTML alongside your application's tests.
https://embermap.github.io/ember-cli-fastboot-testing
MIT License
39 stars 18 forks source link

Environment is "development" instead of "test" during tests #32

Open viniciussbs opened 5 years ago

viniciussbs commented 5 years ago

Hi! I don't know if this is a bug here or maybe on FastBoot - or maybe I am missing something. But this code is evaluating "development" instead of "test":

let config = getOwner(this).resolveRegistration('config:environment');
config.environment // => "development"

This happens only in FastBoot tests. In acceptance tests it evaluates "test".

ryanto commented 5 years ago

Hey @viniciussbs - Quick question: How are you running the tests for you application?

viniciussbs commented 5 years ago

Hi! ember s and then https://localhost:4200/tests?hidepassed, because I was debugging the tests. I was following your tip of running kill -USR1 <pid> to put node into debug mode.

ryanto commented 5 years ago

Ok great. In trying to track this one down I noticed that if you run ember test or ember test --server you'll get the right environment.

I'm waiting to hear back on if we can have the /tests URL use a FastBoot app with a test env.

ryanto commented 5 years ago

Ok quick follow up!

Because you're running ember s and visiting /tests URL you're getting the FastBoot output of a development app, so it has its env set to development. The best way to ensure you have a test env is to use ember test --server.

In order to get this fixed I believe it will require a change to FastBoot. We need a way to tell FastBoot to run with a different environment configuration from what it was built with. My guess is this will require a change to FastBoot itself.

I'm going to close this discussion because I'm not sure there's anything this library can do right now to move this forward. There's an issue over here in FastBoot to track this: https://github.com/ember-fastboot/fastboot/issues/218

viniciussbs commented 5 years ago

Nice! I usually run ember test --server, but at that time I was switching between tests and development, so that's why I was visiting /tests.

Thank you!

jkeen commented 4 years ago

Spent some time trying to track this down and crawled through the fastboot source yesterday, and figured out a way to make this work right now in the fastboot-testing.js setupFastboot hook.

//config/fastboot-testing.js
let config = require('./environment');

module.exports = {
  resilient: false,
  sandboxGlobals: {},
  setupFastboot: fastbootInstance => {
    let oldConfig;
    if (process.env.APP_CONFIG) {
      oldConfig = process.env.APP_CONFIG;
    }

    process.env.APP_CONFIG = JSON.stringify(config('test'));
    fastbootInstance.reload()

    if (oldConfig) {
      process.env.APP_CONFIG = oldConfig;
    }
    else {
      delete process.env.APP_CONFIG
    }
  }
};

Basically what's happening is when the ember app gets built all the current environment config gets stuffed into the package.json file under "AppConfig", which is where it gets read out of when the app gets built in Fastboot. Relevant lines:

https://github.com/ember-fastboot/fastboot/blob/1d0e4c86f4f3bdecc9acc95116c720db17c77a46/src/ember-app.js#L45-L51

So all we need to do from this addon is set the APP_CONFIG environment variable to the stringified configuration we want and reload the fastboot instance so it takes. We can do that in the consumer app's setupFastboot hook, but it's less efficient than it could be because before that hook is called we're calling reload() already from this addon:

https://github.com/embermap/ember-cli-fastboot-testing/blob/7cb4b4705498485dc6c977bf33bbb6b6fc25cf38/lib/helpers.js#L66-68

But this should get @viniciussbs going until a real fix can be put in. And good news: probably no need to modify fastboot? What do you think, @ryanto?

ryanto commented 4 years ago

Oh awesome nice find!

  1. Is process.env.APP_CONFIG public API? Could we box ourselves into a bad situations by overriding that?

  2. The whole setupFastboot hook was added for folks that have really complicated or special FastBoot setups. Does this qualify as a special setup? In other words, how would you feel about fastboot-testing not supporting this out-of-the-box, but instead having instructions in the docs for using /tests?

  3. If we did add this to setupFastboot should we have a way to super it? Or should folks that want to add their own setupFastboot now be responsible for copying over our APP_CONFIG code?

jkeen commented 4 years ago
  1. There's a test for it in the fastboot source and it's preserved in the 3.x fastboot work @rwjblue is doing, so I think it's cool to use?

  2. I don't think this fix constitutes a special setup—rather this is fixing a strange quirk. "I want my application tests to run in development mode but only when I visit my app at localhost:4200/tests and not at localhost:7357" feels like the time you'd say "that's a special setup", in my opinion.

With more people using that mockServer to do full blown acceptance testing with network requests, I'd imagine more people would be perplexed by that issue due to the common pattern of setting env vars for network URLs in the application environment config. The way this bug presented itself to me was the fastboot app loading data from development URLs instead of test data.

  1. What kind of special fastboot setup have you seen people do in that setupFastboot hook? (I ran into another issue on this same project about trying to mock dates using timekeeper but it didn't seem like I could do it in the setupFastboot hook. That's a separate topic)
ryanto commented 4 years ago

Ok cool! I'm on board unless anyone has any objections.

I guess we could move the process.env.APP_CONFIG code to happen as we are setting up Fastboot, so there's no need to reload it in setupFastboot?

Also, once that app is booted should we reset process.env.APP_CONFIG to its original value? I'd imagine ember-cli-fastboot also relies on this code as well, so keeping it as a test env might cause issues?

jkeen commented 4 years ago

Well I spoke too soon and ran into a snag with this hack approach that maybe our fix can overcome?

This setupFastboot hook in fastboot-testing gets run in development mode? When starting up the app in dev mode, the fastboot config has the test envrionment's settings which causes problems. Oof

So I guess we do need to be really careful about that env var in making sure to reset it after it's done what we need it to

jkeen commented 4 years ago

Updated my code snippet above (https://github.com/embermap/ember-cli-fastboot-testing/issues/32#issuecomment-575746204) to account for the above problem and it's working as it should now. Still not sure why that hook loads in dev mode though

ryanto commented 4 years ago

Great!

If we're going to bring this into Fastboot testing does it make sense to move the env logic into this function (https://github.com/embermap/ember-cli-fastboot-testing/blob/master/lib/helpers.js#L73) so that we don't need to call reload?

Any downsides?

jkeen commented 4 years ago

That seems good, I think we might need to do it again before the fastboot.reload() in the reloadServer function too, though, won't we? I think fastboot.reload() rebuilds the app doesn't it?

ryanto commented 4 years ago

Yup nice catch!