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

[FEAT] Add inspect store button to data pane in Ember Inspector #1163

Merged
merged 1 commit into from
Mar 31, 2020

Conversation

SYU15
Copy link
Member

@SYU15 SYU15 commented Mar 25, 2020

Description

Add a button in data pane to enable user to inspect store and send it to console.

Would love some guidance as to what a good test would be since a lot of the end-to-end functionality seems to be stubbed out in other tests.

Screenshots

Data_Pane_Store

@chancancode
Copy link
Member

The code and functionality looks good to me. I'll leave it to @nummi see if there are any UI improvements to suggest. My initial thought is we should incorporate the > $E affordance into the button, maybe in addition to the text label.

Also, do you mind adding a test for this feature?

@SYU15
Copy link
Member Author

SYU15 commented Mar 26, 2020

Also, do you mind adding a test for this feature?

Happy to add a test @chancancode, however I didn't see an example of one that tested things end-to-end (I could have missed it) or if it is ok to stub out a lot of the functionality. Perhaps you could point me in the right direction or provide some guidance?

@nummi
Copy link
Collaborator

nummi commented Mar 26, 2020

How does this look?

image

image


Or this?

image

@SYU15
Copy link
Member Author

SYU15 commented Mar 27, 2020

@nummi @chancancode I updated the button to be the first suggested option. What do you think?
Screen Shot 2020-03-26 at 10 05 50 PM

@chancancode
Copy link
Member

IMO, the $E is important, since it tells you you can find it as that variable in the console, the > alone seems to generic to communicate that, so personally, I like the second option

@SYU15
Copy link
Member Author

SYU15 commented Mar 27, 2020

@chancancode not sure if this is clear in the gif but right now clicking on the inspect button doesn't automatically send the console to the store, it opens up the right-hand panel where the user can click on the $E.
I was following the same pattern as the container pane. Is it problematic to add the $E to the button or is that ok? Or should I just have the button send the store directly to the console?

@chancancode
Copy link
Member

ohhhh sorry I got it all wrong! In that case I agree there is no need for the > $E. My bad!!

@chancancode
Copy link
Member

To be clear, I now think your original design, a button with just the label, is fine, since it doesn't actually send the store to the $E variable. We may want a consistent affordance (icon, or whatever) for "inspecting X", but that would be a separate task since as you said. Sorry for the confusion!

@chancancode
Copy link
Member

For the test, I think you can add an acceptance test in data-test.js, that does something like:

test('...', async function(assert) {
  await visit('/data/model-types');

  respondWith('objectInspector:inspectByContainerLookup', ({ name }) => {
    assert.equal(name, 'service:store');
    return false;
  });

  await click('[data-test-inspect-store]);
});
  1. It visits the data tab.
  2. It sets up the expectation that the objectInspector:inspectByContainerLookup will be sent, with the name argument service:store, and that we are not sending a response (return false).
  3. It clicks the button (which is what triggers the message).
  4. If the message was not sent, the test will fail.

Alternatively, in step 2, we could set it up to send the proper response here (which would be sending the store service object from the "app" to the inspector). However, that would require quite a bit of set up, and then the inspector will send more messages to inspect the properties on the store service object, which, imo, are not really the point of this test.

I think the functionality of the object inspector is already covered in other places, and in here we only really care about the button triggering the object inspector, so I think we can just stop there and trust that the object debug/inspector is going to do the right thing with that message (which is already covered elsewhere).

@SYU15 SYU15 force-pushed the add-store branch 5 times, most recently from 7937092 to db06951 Compare March 31, 2020 02:54
@SYU15 SYU15 marked this pull request as ready for review March 31, 2020 03:14
@SYU15 SYU15 changed the title [FEAT] Add inspect store button to data pane in Ember Inspector (WIP) [FEAT] Add inspect store button to data pane in Ember Inspector Mar 31, 2020
Copy link
Member

@chancancode chancancode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

canary failure is unrelated (see emberjs/ember.js#18839)

@chancancode chancancode merged commit 48c2872 into emberjs:master Mar 31, 2020
chancancode added a commit that referenced this pull request Mar 31, 2020
While working on #1163, we encountered an issue with the test
adapter, where unmet expectations (when you set up a `respondTo`
for a test that was never triggered) did not cause the offending
test to fail. Instead, they became unhandled promise rejections
(i.e. "global errors") and fails the _next_ test that runs.

This is because we were using the `QUnit.{testStart,testDone}`
hooks as if they were a global version of `{before,after}Each`.
However, the global hooks are really intended for things like
test reporters and runs too late to affect things like the pass
or fail status of a test.

This commit introduces a `setupTestAdapter` function that uses
the normal (not global) APIs so that the failures are attached
to the correct test.
chancancode added a commit that referenced this pull request Mar 31, 2020
While working on #1163, we encountered an issue with the test
adapter, where unmet expectations (when you set up a `respondTo`
for a test that was never triggered) did not cause the offending
test to fail. Instead, they became unhandled promise rejections
(i.e. "global errors") and fails the _next_ test that runs.

This is because we were using the `QUnit.{testStart,testDone}`
hooks as if they were a global version of `{before,after}Each`.
However, the global hooks are really intended for things like
test reporters and runs too late to affect things like the pass
or fail status of a test.

This commit introduces a `setupTestAdapter` function that uses
the normal (not global) APIs so that the failures are attached
to the correct test.
chancancode added a commit that referenced this pull request Mar 31, 2020
While working on #1163, we encountered an issue with the test
adapter, where unmet expectations (when you set up a `respondTo`
for a test that was never triggered) did not cause the offending
test to fail. Instead, they became unhandled promise rejections
(i.e. "global errors") and fails the _next_ test that runs.

This is because we were using the `QUnit.{testStart,testDone}`
hooks as if they were a global version of `{before,after}Each`.
However, the global hooks are really intended for things like
test reporters and runs too late to affect things like the pass
or fail status of a test.

This commit introduces a `setupTestAdapter` function that uses
the normal (not global) APIs so that the failures are attached
to the correct test.
nummi pushed a commit to nummi/ember-inspector that referenced this pull request Apr 5, 2020
nummi pushed a commit to nummi/ember-inspector that referenced this pull request Apr 5, 2020
While working on emberjs#1163, we encountered an issue with the test
adapter, where unmet expectations (when you set up a `respondTo`
for a test that was never triggered) did not cause the offending
test to fail. Instead, they became unhandled promise rejections
(i.e. "global errors") and fails the _next_ test that runs.

This is because we were using the `QUnit.{testStart,testDone}`
hooks as if they were a global version of `{before,after}Each`.
However, the global hooks are really intended for things like
test reporters and runs too late to affect things like the pass
or fail status of a test.

This commit introduces a `setupTestAdapter` function that uses
the normal (not global) APIs so that the failures are attached
to the correct test.
patricklx pushed a commit to patricklx/ember-inspector that referenced this pull request Sep 19, 2022
patricklx pushed a commit to patricklx/ember-inspector that referenced this pull request Sep 19, 2022
While working on emberjs#1163, we encountered an issue with the test
adapter, where unmet expectations (when you set up a `respondTo`
for a test that was never triggered) did not cause the offending
test to fail. Instead, they became unhandled promise rejections
(i.e. "global errors") and fails the _next_ test that runs.

This is because we were using the `QUnit.{testStart,testDone}`
hooks as if they were a global version of `{before,after}Each`.
However, the global hooks are really intended for things like
test reporters and runs too late to affect things like the pass
or fail status of a test.

This commit introduces a `setupTestAdapter` function that uses
the normal (not global) APIs so that the failures are attached
to the correct test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants