fix SCT-2914 / PUBLIC-1493 - movement of unselected cells #2076
Conversation
- fix movement of unselected cells - improve styling of toolbar controls in unselected cells - integration tests for the above
|
This pull request fixes 1 alert when merging 6aad8e9 into bcd0423 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 632318d into bcd0423 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging a4a9a83 into bcd0423 - view on LGTM.com fixed alerts:
|
Codecov Report
@@ Coverage Diff @@
## develop #2076 +/- ##
===========================================
- Coverage 23.63% 18.11% -5.53%
===========================================
Files 515 462 -53
Lines 61621 48767 -12854
===========================================
- Hits 14567 8836 -5731
+ Misses 47054 39931 -7123
Continue to review full report at Codecov.
|
|
This pull request fixes 1 alert when merging 5ae04bc into bcd0423 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 9b60725 into bcd0423 - view on LGTM.com fixed alerts:
|
|
This should be pretty much ready to go. New and old integration tests pass for ci and narrative-dev. |
| toggleIcon = (toggleMinMax === 'maximized' ? 'minus' : 'plus'), | ||
| color = (toggleMinMax === 'maximized' ? '#000' : 'rgba(255,137,0,1)'); | ||
| classModifier = (toggleMinMax === 'maximized' ? '-maximized' : '-minimized'); |
| style: { | ||
| color: color | ||
| } | ||
| class: 'fa fa-' + toggleIcon + '-square-o fa-lg' |
There was a problem hiding this comment.
fa fa-${toggleIcon}-square-o fa-lg?
There was a problem hiding this comment.
This was already in place, not changed by the PR. The change was to remove the explicit color style.
But maybe I don't know what the question is?
The toggleIcon in this case is the functional part of the font awesome class name, which in this case is either fa-minus-square-o or fa-plus-square-o , so basically toggles between the minus and plus icons.
I was tempted to, but did not, improve that line by making it a template string.
There was a problem hiding this comment.
The comment was just about using a template string. ATM we don't have any policies either way so it's up to you. 🙂
There was a problem hiding this comment.
Oh, right, well, I have tried to restrain myself from just changing code to be better. I do try to restrain myself to only changing code if
(a) it is required for the fix
(b) it is a latent bug not described in the bug report
(c) it is seriously required to have to code make sense (a big exception is any jQuery code, which never makes sense.)
(d) it is required for a test or makes a test easier or more reliable, e.g. an attribute to select on
(e) it angers eslint
I'm sure there are others ...
That said, I do sometimes just make code improvements if they bug me enough and I'm in a generous mood.
In this case, string concatenation is old fashioned, but there are literally hundreds of not thousands of locations in the codebase where they are used and can be transformed into template strings :)
It sure is satisfying to do it, though :)
There was a problem hiding this comment.
(c) - 😁
Template strings can be implemented globally by eslint, should we wish to adopt a policy towards them at some point.
| } | ||
| }; | ||
|
|
||
| const testCases = mergeObjects([testData[browser.config.testParams.ENV], testData.all]); |
There was a problem hiding this comment.
Wouldn't it be simpler to do something like:
const narrativeIdsByEnv = {
ci: 58675,
'narrative-dev': 80970,
},
narrativeEnv = browser.config.testParams.ENV;
if (!narrativeIdsByEnv[narrativeEnv]) {
// bail early
}
const narrativeId = narrativeIdsByEnv[narrativeEnv];
// in the tests
it('moves a minimized selected cell down', async () => {
const testCase = testCases.TEST_CASE1;
await login();
const narrativeContainer = await openNarrative(narrativeId);
// etc.?
There was a problem hiding this comment.
An update to the PR will include a general solution to this.
I had punted on this, because I wanted to keep the test data confined to a single json-compatible structure -- probably better to keep the test data in a file at some point, as it can get pretty large for some test cases, and keeping it in the set file, while simpler for now, makes the test files bigger.
There was a problem hiding this comment.
Thinking about this a bit more and looking at the other existing integration tests (which aren't in the truss / bulk import cell branch yet), I think it would make sense to have a utility function to get the narrative ID for a given test in a certain environment, and to skip the tests if there isn't a narrative available for that test in that env. Something like
// in a utils file
function getTestNarrativeId(testId) {
const tests = {
kbaseCellToolbarMenu: {
ci: 58675,
'narrative-dev': 80970,
},
kbaseCellSidebarMenu: {
ci: 12345,
},
...
};
const env = browser.config.testParams.ENV;
if (!tests[testId]) {
throw new Error(`Invalid testId ${testId}`);
}
if (!tests[testId][env]) {
return null;
}
return tests[testId][env];
}// in the test file
const testId = 'kbaseCellToolbarMenu'; // could derive dynamically from the file name
const narrativeId = testUtils.getTestNarrativeId(testId);
if (!narrativeId) {
// skip the tests - no narrative available in this env
}That would save from having repeated boilerplate in each integration test.
(Going a step further, you could enclose the tests in a function, and have a runTests util function to which you pass the test ID and the test function; runTests could either execute the function (thereby running the tests) or could skip that test set if there was no narrative available.)
There was a problem hiding this comment.
The idea was to eventually move all test data into a separate json file, so they all live in a single test data object.
The test data has since been moved into a companion test data file.
There was a problem hiding this comment.
I'm sure the integration test support will continually evolve. There are few enough tests that it is easy to refactor them all in a few minutes when we do change that support.
Since the original PR, I refactored the narrative-centric util methods into a NarrativeTesting class, which knows about the structure of test data utilized by the tests I've written. An instance is created for each test, given the test data, a test case key, the environment. Some things are assumed to be in the global test config, which I'm not comfortable with ... but it is a process.
(BTW some of my comments may have come after yours but not reflect them, because this page was stale.)
Actually, globally, I'm not sure that this is the best testing framework to use anyway. It is actually quite easy to create false positive test just by missing an await ... the promises are uncontrolled, and the test completes without error, although the promises may actual resolve to an error. It is asking a lot for devs to remember when they need to use await (which is just about always when using the WebdriverIO api).
I found earlier that the synchronous tests did not work well, especially if one needs to wrap them and mix in custom test code, but that may have changed since, and I may not have tried hard enough.
| async function selectCellWithBody(container, cellIndex, body) { | ||
| const cell = await waitForCellWithBody(container, cellIndex, body); | ||
|
|
||
| // Make sure not selected. | ||
| // We do this by inspecting the border. | ||
| await browser.waitUntil(async () => { | ||
| const borderStyle = await cell.getCSSProperty('border'); | ||
| return borderStyle.value === testData.common.unselectedBorder; | ||
| }); | ||
|
|
||
|
|
||
| // Click on title area to select cell. | ||
| const bodyArea = await cell.$('.text_cell_render.rendered_html'); | ||
| await clickWhenReady(bodyArea); | ||
|
|
||
| await browser.waitUntil(async () => { | ||
| const borderStyle = await cell.getCSSProperty('border'); | ||
| return borderStyle.value === testData.common.selectedBorder; | ||
| }); | ||
|
|
||
| return cell; | ||
| } |
There was a problem hiding this comment.
This is almost identical to the function above. I would refactor this code to create a more general selectCellPart function, which has an additional param that indicates whether the title or the body should be selected.
There was a problem hiding this comment.
There will be an update to this. Not quite the way you suggested, but close in spirit.
| await waitForCellWithTitle(narrativeContainer, testCase.cellIndex - 1, testCase.title); | ||
| }); | ||
|
|
||
| // Everything above, but cells are expanded. |
There was a problem hiding this comment.
Each test starts with await login(), so that can go into the beforeEach() function.
If you're repeating a set of tests with a single variable changed, I would rewrite my testing functions to parameterise the variable; it makes the code easier to maintain and there's less chance of a copy-paste error. ISTM you could write a more generic testing function that takes in four params--expanded/collapsed state, selection state, and movement direction, plus test case data--and use that to test all combinations of parameters.
There was a problem hiding this comment.
I did quite a round of test refactoring to be more DRY, and fixed some false test positives.
Basically, I was getting stretched by the amount of test time to actual bug fixing time, and just stopped when the tests reported success (and a round or two of refactoring them.) That is partly because the integration testing is so new, and we are still building up support and getting experience.
…ogin() into beforeEach()
- could be handy to obscure in CI, but expose locally.
…ompartmentalize usage of test data.
- improve NarrativeTesting tools - add range function to wdio utils - public tab adjust to NarrativeTesting chagnes - rarrange tests to run through single cell movement function, with rearranged test data.
|
This pull request fixes 1 alert when merging 25ef6fe into 4e74893 - view on LGTM.com fixed alerts:
|
|
SonarCloud Quality Gate failed.
|
|
This pull request fixes 7 alerts when merging 11c8619 into e38ffd5 - view on LGTM.com fixed alerts:
|
| div( | ||
| { | ||
| style: | ||
| 'display:inline-block; width: 34px; vertical-align: top;', |
There was a problem hiding this comment.
can all these inline styles be moved into the relevant stylesheet?
There was a problem hiding this comment.
Sure, but the goal was not to make as few code changes as possible, whilst adding testing attributes.
There was a problem hiding this comment.
Yes, they could be. But the point of changing that file at all was to make a small change to allow it to be testable, not to refactor it.
| @@ -622,8 +523,9 @@ define([ | |||
| } | |||
|
|
|||
| return { | |||
| make: function (config) { | |||
| return factory(config); | |||
| // eslint-disable-next-line no-unused-vars | |||
There was a problem hiding this comment.
why not just remove the _config?
There was a problem hiding this comment.
Just making eslint happy, whilst preserving the original intent (to communicate that a config object is provided). Now removed.
package.json
Outdated
| "eslint-check": "eslint '**/*.js'", | ||
| "eslint-pre-push": "scripts/eslint-pre-push.sh", |
There was a problem hiding this comment.
could you move these up so they're under the eslint item?
There was a problem hiding this comment.
I've alphabetized the list, which reordered other entries as well.
…tes a bunch of additional warnings, but ignoring.
|
This pull request fixes 27 alerts when merging 1749f9a into dbcbd63 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 27 alerts when merging a6ddfa6 into dbcbd63 - view on LGTM.com fixed alerts:
|
…medriver and chrome-on-host.
|
SonarCloud Quality Gate failed.
|
|
This pull request fixes 27 alerts when merging 68536e5 into dbcbd63 - view on LGTM.com fixed alerts:
|
|
@briehl I think integration tests are failing due to an expired token. |
|
Just went through a [insert favorite adjective] round of merge conflicts. Hopefully all is well afterwards. |
|
This pull request fixes 7 alerts when merging d5febdd into 8769a64 - view on LGTM.com fixed alerts:
|
|
SonarCloud Quality Gate failed.
|
|
This pull request fixes 7 alerts when merging 0d7876c into 8769a64 - view on LGTM.com fixed alerts:
|
|
SonarCloud Quality Gate failed.
|
|
This pull request fixes 7 alerts when merging 820f334 into e628c87 - view on LGTM.com fixed alerts:
|
Description of PR purpose/changes
Fixes an issue with using the up and down arrow cell movement controls in unselected cells. Rather than the the unselected moving, the selected cell moves.
In addition, the styling of the toolbar controls in unselected cells is a bit too dim, and they don't respond well to hovering.
The fix for the cell movement is to have the cell movement commands include the index of the target (unselected) cell. The styling issue was a matter of adding classes to better support unselected cell styles, and altering the styles to make the controls a bit darker (they were double-subdued by being both a gray color and also having low opacity applied, which was probably unintentional), and in addition when hovering they appear as in selected cells (full color, background color applied.)
To support integration tests, some additional markup changes were required.
Jira Ticket / Issue
original: https://kbase-jira.atlassian.net/browse/PUBLIC-1493
work tracking: https://kbase-jira.atlassian.net/browse/SCT-2914
Testing Instructions
The changes are covered by the integration tests in kbaseCellToolbarMenu_test.js
Dev Checklist:
Updating Version and Release Notes (if applicable)