Skip to content
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

Allow addons to inject middleware into testem #2128

Merged
merged 1 commit into from
Oct 6, 2014
Merged

Allow addons to inject middleware into testem #2128

merged 1 commit into from
Oct 6, 2014

Conversation

bcardarella
Copy link
Contributor

This PR depends upon testem/testem#410 being
accepted before it should be pulled in

Primarily the use case is with any addon that injects a middleware into
the ember server's, this will allow addon authors to also expose that
middleware through ember test if/when noted PR above is accepted by
@airportyh

bcardarella added a commit to DavyJonesLocker/ember-cli-proxy-fixtures that referenced this pull request Sep 27, 2014
@rwjblue
Copy link
Member

rwjblue commented Sep 27, 2014

Seems somewhat odd that we would have two different middleware mechanisms.

  • Testem already has the ability to proxy requests to a backend server.
  • We already have a way to start a server with middlewares supplied by addon's.

Seems like we could put those two things without adding a new hook or requiring changes to testem.

@bcardarella
Copy link
Contributor Author

@rwjblue I disagree. This allows addon authors to specifically target middlewares for each environment. There are many middlewares that don't need to run in testem.

@rwjblue
Copy link
Member

rwjblue commented Sep 27, 2014

The middlewares already receive an options hash (which can be used to only add middlewares in a given environment for example). Also, running the ember test commands set an environment variable (and the command itself should be in that options hash).

I think this implementation is good, I just generally do not think we need to support two similar but different API's for roughly the same thing.

@bcardarella
Copy link
Contributor Author

Managing one middleware stack for two separate express servers is going to be a huge PITA. Rather than a clean API you're suggesting adding complexity to existing middleware functions to support different environments or conditionally turn on/off middleware when necessary. That feels odd to me.

@bcardarella
Copy link
Contributor Author

Furthermore, this is already done.

@rwjblue
Copy link
Member

rwjblue commented Sep 27, 2014

Point of clarification: I am suggesting using a single express server (ours), this solution suggests using two (our for some middlewares and testems for others).

@rwjblue
Copy link
Member

rwjblue commented Sep 27, 2014

The doneness however is a wonderful point. 😃

@bcardarella
Copy link
Contributor Author

My argument could be completely moot if testem/testem#410 fails to get merged

@bcardarella
Copy link
Contributor Author

@rwjblue testem/testem#410 has been merged and testem 0.6.22 has been released. Any issues with this merging?

@stefanpenner
Copy link
Contributor

@bcardarella travis is emo, otherwise not at all.

This PR depends upon testem/testem#410 being
accepted

Primarily the use case is with any addon that injects a middleware into
the ember server's, this will allow addon authors to also expose that
middleware through `ember test` if/when noted PR above is accepted by
@airportyh
bcardarella added a commit that referenced this pull request Oct 6, 2014
Allow addons to inject middleware into testem
@bcardarella bcardarella merged commit b7ff379 into ember-cli:master Oct 6, 2014
@bcardarella bcardarella deleted the bc-support-testem-middleware branch October 6, 2014 19:55
@bcardarella
Copy link
Contributor Author

Bomb's away!

@stefanpenner
Copy link
Contributor

@bcardarella !!!

@hparra
Copy link
Contributor

hparra commented Oct 23, 2014

I'm a little confused about how this PR can(not) solve my use case.

I would like ember test, with or without --server, to also run the proxy-server addon. This all works fine when I'm in http://localhost:4200/tests while running ember server.

In attempting to figure this out I noticed that this.addonMiddlewares() in lib/tasks/test.js returns an empty array. Am I missing something?

@stefanpenner
Copy link
Contributor

i don't believe the full machinery to support this is implemented yet. I would like to suggest we unified the test runner, and merely via configuration tell it to run once or run continuously (ala --server) this would allow improvements to either code path to immediately improve the other.

jakecraige pushed a commit to jakecraige/ember-cli-proxy-fixtures that referenced this pull request Nov 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants