- 13 Aug, 2020 40 commits
-
-
Mark Florian authored
Some Geo tests needed to be fixed to support this version bump. Details below. For the GeoNodeFormCapacities and GeoNodeFormCore components: The tests set the input's value to the empty string, and try to assert that the form displays an error message saying that the input can't be empty. This doesn't work, because the underlying `BFormInput` does not emit a model event in the case that the model value hasn't changed (the value starts as an empty string, and ends up as an empty string!). Prior to this, the tests were passing because the `input` event is not the model event as far as the `BFormInput` is concerned, which it always fires. So, the fix in this case is to do one of the following: 1. Listen for the update event in the component, rather than the input event. 2. In the test, set the input element's value to something else before making it the empty string, so the model event fires. The first option seems best, since it's more "correct", but this change is entirely for the sake of the tests. Real-world behaviour won't have changed at all either way. This is because there's no way for a user to set an input to the empty string when it's already the empty string, so this edge case can't happen in reality. For the GeoSettingsForm component: This is actually a very subtle confluence of bugs, including implicit type casting, unrealistic synchronous calls and inconsistent amounts of behaviour mocking. Individually these are harmless, but all together produced a failing test. The crux of this is that `BFormInput` emits a model (`update`) Vue event when a [`blur` DOM event is triggered][237] on the input element, **if** the [_formatted_ input value doesn't strictly equal the `model` value][161]. The _formatted_ value is [_stringified_][138] copy of the model value (unless the `formatter` prop is given), which means that when the model is a `number`, the `model` ends up getting overwritten, because a string can't equal a number. Prior to GitLab UI v18.7, that model (`update`) event was completely ignored, and so triggering the `blur` *didn't* re-update the model value with the old value. But _with_ GitLab UI v18.7, that model (`update`) event _is_ listened to, (via the remapping to the `input` event) and the model gets is updated accordingly _because_ of the blur. The reason the _old_ value is used by `BFormInput`'s `onBlur` method is because it is called _synchronously_ after emitting the model event. This means that the parent scope's model has been updated, but Vue hasn't _yet_ passed it down to the component again. If you `await wrapper.vm.$nextTick()` _before_ triggering the blur, the _new but stringified_ value will be in the parent scope's model instead of the _old_ stringified model value. (This is was [suggested][zcuddy] by @zcuddyy, although _technically_ the model would a string instead of a number with this approach.) Finally, this can be reproduced with `BFormInput` itself. Below are some tests that simplify and capture the problem with the way the Geo tests were written: ```js import { BFormInput } from 'bootstrap-vue'; import { mount } from '@vue/test-utils'; describe('number/text confusion', () => { const factory = (template, value) => { wrapper = mount({ components: { BFormInput }, data: () => ({ modelValue: value }), template, }); }; describe('given a number value without number prop', () => { beforeEach(() => { factory(`<b-form-input v-model="modelValue" />`, 5); }); it('blur changes the model value to stringified original', () => { expect(wrapper.vm.modelValue).toBe(5); // BFormInput's model event is 'update' wrapper.find(BFormInput).vm.$emit('update', 20); expect(wrapper.vm.modelValue).toBe(20); wrapper.find('input').trigger('blur'); // Note value has changed! expect(wrapper.vm.modelValue).toBe('5'); }); }); describe('given a number value with number prop', () => { beforeEach(() => { factory(`<b-form-input v-model="modelValue" number />`, 5); }); it('blur does not mess things up', () => { expect(wrapper.vm.modelValue).toBe(5); // BFormInput's model event is 'update' wrapper.find(BFormInput).vm.$emit('update', 20); expect(wrapper.vm.modelValue).toBe(20); wrapper.find('input').trigger('blur'); // Note value has *not* changed expect(wrapper.vm.modelValue).toBe(20); }); }); }); ``` This explains why doing `findGeoSettingsTimeoutField().vm.$emit('blur');` fixes the tests (because this bypasses `BFormInput`'s `onBlur` handler), and why `findGeoSettingsTimeoutField().setValue(data);` also fixes the tests (because it correctly updates the _internal_ `vModelValue` of the `BFormInput`, such that the `onBlur` handler uses that new value rather than the old one). Phew! What all this means that this is not a bug in GitLab UI v18.7, and it's not a bug in `bootstrap-vue`. It's *actually* a subtle bug in `GeoSettingsForm`, due to weak typing of the model and our previously broken `GlFormInput` model binding that GitLab UI v18.7 fixes. Therefore, I think the _correct_ fix here is two-fold: 1. set the `number` prop to `true` on the `timeout` field 1. Use `setValue` in _both_ tests Either one of these fixes the tests, but I think *both* should be done. The first correctly casts the model to a number, and the second more closely mirrors a user's action. [138]: https://github.com/bootstrap-vue/bootstrap-vue/blob/028b880ea1ec168888368f3abc7e89f1f8f2f9cd/src/mixins/form-text.js#L138 [161]: https://github.com/bootstrap-vue/bootstrap-vue/blob/028b880ea1ec168888368f3abc7e89f1f8f2f9cd/src/mixins/form-text.js#L161 [237]: https://github.com/bootstrap-vue/bootstrap-vue/blob/028b880ea1ec168888368f3abc7e89f1f8f2f9cd/src/mixins/form-text.js#L237 [zcuddy]: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38824#note_394960100
-
Russell Dickenson authored
Add docs for Redis HLL counters Closes #235457 See merge request gitlab-org/gitlab!39298
-
Alina Mihaila authored
-
Nicolò Maria Mezzopera authored
Add visual error handling to MR analytics Throughput chart See merge request gitlab-org/gitlab!39196
-
Olena Horal-Koretska authored
Remove refresh button css in environments Closes #208205 See merge request gitlab-org/gitlab!39305
-
Achilleas Pipinellis authored
Add extra troubleshooting steps when promoting a Geo secondary See merge request gitlab-org/gitlab!39274
-
Catalin Irimie authored
-
Kushal Pandya authored
Disable loading hasNextPage value for Security Dashboards See merge request gitlab-org/gitlab!39261
-
Alan (Maciej) Paruszewski authored
This change disables loading hasNextPage for Vulnerabilities as this is causing Internal Server Error caused by timeouts on database. This is the temporary solution to solve that problem.
-
Kushal Pandya authored
Swimlanes - Fetch issues using GraphQL endpoint See merge request gitlab-org/gitlab!39054
-
Kushal Pandya authored
Add runbook to metric dropdown See merge request gitlab-org/gitlab!39288
-
Tristan Read authored
-
Achilleas Pipinellis authored
Remove mentions of CC from version notes See merge request gitlab-org/gitlab!39326
-
Florie Guibert authored
- Fetch issues for all board lists though VueX action
-
Miguel Rincon authored
Doc update - alert runbooks See merge request gitlab-org/gitlab!38661
-
Tristan Read authored
-
Miguel Rincon authored
Remove the `alert_runbooks` feature flag See merge request gitlab-org/gitlab!39315
-
Tristan Read authored
-
Evan Read authored
Minor spelling update See merge request gitlab-org/gitlab!39417
-
Ezekiel Kigbo authored
-
Sanad Liaquat authored
Stablize transfer project spec by waiting for confirm button enabled Closes #218302 See merge request gitlab-org/gitlab!39324
-
Kushal Pandya authored
Add group milestones API method to api.js Closes #232520 See merge request gitlab-org/gitlab!39280
-
Ramya Authappan authored
Add retry to work around a seldom inconsistent UI issue Closes #231242 See merge request gitlab-org/gitlab!39333
-
Ramya Authappan authored
Rename Gitaly HA to Gitaly Cluster in E2E tests Closes gitlab-org/quality/team-tasks#573 See merge request gitlab-org/gitlab!38251
-
Mark Lapierre authored
The two specs were very similar, the failover spec only lacked the reconciliation step.
-
charlie ablett authored
Fix typo for area chart embeds See merge request gitlab-org/gitlab!39392
-
Miguel Rincon authored
Add Commented by to MR widget Closes #10294 See merge request gitlab-org/gitlab!38136
-
Mayra Cabrera authored
Update metric dashboard validator See merge request gitlab-org/gitlab!38847
-
Ryan Cobb authored
Update metric dashboard validator to use dashboard path
-
Miguel Rincon authored
Optimize click area in design version dropdown See merge request gitlab-org/gitlab!38747
-
Ezekiel Kigbo authored
Migrate merge immediately dialog away from deprecated button Closes #219829 See merge request gitlab-org/gitlab!39284
-
Evan Read authored
-
Suzanne Selhorn authored
Add job tokens example to nuget docs Closes #36424 See merge request gitlab-org/gitlab!39317
-
Giorgenes Gelatti authored
-
Ezekiel Kigbo authored
Replace fa-info-circle icons with GitLab SVG information icon Closes #225948 See merge request gitlab-org/gitlab!38505
-
Paul Gascou-Vaillancourt authored
-
Douglas Barbosa Alexandre authored
Do not render k8s feature highlight if not necessary Closes #234087 See merge request gitlab-org/gitlab!39228
-
Enrique Alcántara authored
Avoid rendering k8s feature highlight when the operations section in the sidebar is not active
-
Douglas Barbosa Alexandre authored
Redirect to new metrics dashboard page See merge request gitlab-org/gitlab!38364
-
Douglas Barbosa Alexandre authored
Ignore requirements test reports pipeline_id column See merge request gitlab-org/gitlab!39366
-