-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(ui): tweak telegraf nginx and redis flows #15710
The head ref may contain hidden characters: "bs_fix_nginx_wizard_\u{1F9D9}\u200D"
Conversation
fd10013
to
50f99cd
Compare
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.
some curiosity about the test setup, but golf claps and raises all around
AutoComplete, | ||
Columns, |
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.
WHOOOO alphabetical!!!
ui/cypress/e2e/collectors.test.ts
Outdated
// fix for https://github.com/influxdata/influxdb/issues/15500 | ||
describe('configuring nginx', () => { | ||
beforeEach(() => { | ||
cy.contains('Create Configuration').click() |
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.
why this and not just cy.visit(whateverurl)
? do we want to test that you always get to the page the same way? or is this some weirdness with the tests? also, idk what i'm reading
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 all happens in the context of a popups / modals - you click 'Create Configuration', then you click NGINX (or redis), then Cotinue, etc. i'll add a comment explaining it.
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.
might be fine as is. i was just under the impression that all of our modals are currently behind a route and got suspicious about state management within the app.
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.
@drdelambre depending on the url you are trying to visit since it might be outside of our control. Generally, if we're trying to test out whether or not we are visiting that other page (i.e. within our site) it's just easy to test out that other page separately rather than include that within the scope of this test
@@ -77,7 +77,7 @@ describe('Onboarding.Components.ConfigureStep.Streaming.ConfigFieldSwitcher', () | |||
const {wrapper} = setup({fieldName, fieldType, value}, true) | |||
|
|||
const input = wrapper.find(ArrayFormElement) | |||
const formElement = wrapper.find(FormElement) | |||
const formElement = wrapper.find(FormElement).first() |
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.
👍
50f99cd
to
f5d70db
Compare
|
||
it('can add and delete urls', () => { | ||
cy.getByTestID('input-field').type('http://localhost:9999') | ||
cy.contains('Add').click() |
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.
How would you feel about checking to see if the input-field was cleared after the click resolved?
Closes #15500
This wasn't actually a bug, but a confusing interaction. This uri array component expects users to hit enter to add an item to the list (good luck, mobile / tablet users). This adds a button to help out that interaction. It also changes the way the form is handled so that it doesn't fire an empty event, causing an error.
The bug was reported for nginx, but the confusing interaction also appeared in redis, so this fix has been applied there as well. e2e tests were added to both.