-
Notifications
You must be signed in to change notification settings - Fork 292
move ember debug tests into own app #2642
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: main
Are you sure you want to change the base?
Conversation
ea8f7dd to
c38d0f0
Compare
|
Since this PR changes the usage of |
1df0de2 to
24127a0
Compare
ed1d1b6 to
f15771c
Compare
ae33cb7 to
b085545
Compare
| <script src="{{rootURL}}assets/vendor.js"></script> | ||
| <script src="{{rootURL}}assets/test-support.js"></script> | ||
| <script src="{{rootURL}}assets/test-app.js"></script> | ||
| <script src="{{rootURL}}ember_debug.js"></script> |
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.
here we inject ember debug into the test app
de7b45d to
c238b51
Compare
7d97550 to
2bf5316
Compare
BlueCutOfficial
left a comment
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.
To be fair I don't understand 100% all the details of all the changes so I would use at least one second opinion (cc @mansona) but this iteration looks fine IMO. The general approach to separate UI tests and ember_debug tests makes sense to me + fixing all the ember-try scenarios recreates more confidence in the test suite.
(Purely on the form, I think enabling 6+ and fixing 3.16~3.24 could have been done in 2 separated PRs, but I woundn't consider this a blocker since the commit stack is clean)
985be8c to
77df22c
Compare
8be8b8b to
6659f91
Compare
6659f91 to
850d0a8
Compare
|
Hi @patricklx, I've an update about this PR. @mansona has initiated a task to improve the "start wrapper" part of the inspector, based on his work on Rollup. This would enable the possibility of importing an adapter, so we wouldn't have to re-implement an adapter in each test app. We suggest keeping the present PR on hold until Chris has finished that work, then we can rebase and include this improvement. |
850d0a8 to
a13f486
Compare
| nodeResolve(), | ||
| commonjs(), | ||
| // eslint-disable-next-line no-undef | ||
| process.env.NO_DEL_DIST && del({ targets: 'dist' }), |
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.
this is for build watch mode.
we want to pre-build ember-debug
and then start build watch for ember-debug and inspector-ui.
ember-cli-build.js depends on dist output of ember-debug.
but when both watches are started, dist gets removed and the build watch for inspector-ui fails
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.
a copy of inspector-ui app service to setup communication with ember-debug
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.
a copy of inspector-ui app service to setup communication with ember-debug
e93965d to
a5cfae1
Compare
a5cfae1 to
f673d10
Compare
|
@BlueCutOfficial @mansona has this addressed the previous comment about rebasing on the other adapter work? |
ember debug is the thing that should/can be tested with older ember versions.
for inspector ui its not required. we can extend it for it as well though.