From a97d9be21028e574a2c6498fcd6a1909e06303ff Mon Sep 17 00:00:00 2001 From: Chris Breiding Date: Thu, 12 Jul 2018 18:15:07 -0400 Subject: [PATCH] Fix screenshot paths differing between interactive and run modes (#2103) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor desktop-gui spec path handling - differentiate folders vs spec files - simplify specs store, remove mutations - fix spec changing via browser url - normalize paths for windows on server * move windows paths logic back to desktop-gui using any method from node’s path util converts / back to \ on windows, so trying to normalize the paths to use / is futile. instead, properly split and compare paths in the desktop gui as needed --- .../integration/specs_list_spec.coffee | 4 +- .../desktop-gui/src/projects/projects-api.js | 6 +- .../desktop-gui/src/specs/folder-model.js | 21 +++ packages/desktop-gui/src/specs/spec-model.js | 37 ++--- packages/desktop-gui/src/specs/specs-list.jsx | 34 ++-- packages/desktop-gui/src/specs/specs-store.js | 148 +++++++----------- 6 files changed, 105 insertions(+), 145 deletions(-) create mode 100644 packages/desktop-gui/src/specs/folder-model.js diff --git a/packages/desktop-gui/cypress/integration/specs_list_spec.coffee b/packages/desktop-gui/cypress/integration/specs_list_spec.coffee index 35788a8327..340688daa6 100644 --- a/packages/desktop-gui/cypress/integration/specs_list_spec.coffee +++ b/packages/desktop-gui/cypress/integration/specs_list_spec.coffee @@ -330,9 +330,7 @@ describe "Specs List", -> cy.get("@firstSpec").should("not.have.class", "active") cy.get("@secondSpec").should("have.class", "active") - ## We aren't properly handling this event so skipping - ## this test for now until its implemented - describe.skip "spec list updates", -> + describe "spec list updates", -> beforeEach -> @ipc.getSpecs.yields(null, @specs) @openProject.resolve(@config) diff --git a/packages/desktop-gui/src/projects/projects-api.js b/packages/desktop-gui/src/projects/projects-api.js index a264b0a583..37f37931aa 100644 --- a/packages/desktop-gui/src/projects/projects-api.js +++ b/packages/desktop-gui/src/projects/projects-api.js @@ -68,7 +68,7 @@ const runSpec = (project, spec, browser) => { const launchBrowser = () => { project.browserOpening() - ipc.launchBrowser({ browser, spec: spec.obj }, (err, data = {}) => { + ipc.launchBrowser({ browser, spec: spec.file }, (err, data = {}) => { if (data.browserOpened) { project.browserOpened() } @@ -149,8 +149,8 @@ const openProject = (project) => { viewStore.showProjectSpecs(project) }) - ipc.onSpecChanged((__, spec) => { - specsStore.setChosenSpecByRelativePath(spec) + ipc.onSpecChanged((__, relativeSpecPath) => { + specsStore.setChosenSpecByRelativePath(relativeSpecPath) }) ipc.onConfigChanged(() => { diff --git a/packages/desktop-gui/src/specs/folder-model.js b/packages/desktop-gui/src/specs/folder-model.js new file mode 100644 index 0000000000..ddeb48d648 --- /dev/null +++ b/packages/desktop-gui/src/specs/folder-model.js @@ -0,0 +1,21 @@ +import { action, computed, observable } from 'mobx' + +export default class Directory { + @observable path + @observable displayName + @observable isExpanded = true + @observable children = [] + + constructor ({ path, displayName }) { + this.path = path + this.displayName = displayName + } + + @computed get hasChildren () { + return this.children.length + } + + @action setExpanded (isExpanded) { + this.isExpanded = isExpanded + } +} diff --git a/packages/desktop-gui/src/specs/spec-model.js b/packages/desktop-gui/src/specs/spec-model.js index 95531bd80b..f87d307bdd 100644 --- a/packages/desktop-gui/src/specs/spec-model.js +++ b/packages/desktop-gui/src/specs/spec-model.js @@ -1,39 +1,28 @@ import _ from 'lodash' -import { action, observable } from 'mobx' +import { computed, observable } from 'mobx' export default class Spec { - @observable name @observable path + @observable name + @observable absolute @observable displayName + @observable type @observable isChosen = false - @observable isExpanded = false - @observable children = [] - constructor ({ obj, name, displayName, path }) { - this.obj = obj - this.name = name + constructor ({ path, name, absolute, relative, displayName, type }) { this.path = path - this.isExpanded = true + this.name = name + this.absolute = absolute + this.relative = relative this.displayName = displayName + this.type = type } - getStateProps () { - return _.pick(this, 'isChosen', 'isExpanded') + @computed get hasChildren () { + return false } - hasChildren () { - return this.children && this.children.length - } - - @action merge (other) { - _.extend(this, other.getStateProps()) - } - - @action setChosen (isChosen) { - this.isChosen = isChosen - } - - @action setExpanded (isExpanded) { - this.isExpanded = isExpanded + @computed get file () { + return _.pick(this, 'name', 'absolute', 'relative') } } diff --git a/packages/desktop-gui/src/specs/specs-list.jsx b/packages/desktop-gui/src/specs/specs-list.jsx index b9aa6b5bbb..e763a5747d 100644 --- a/packages/desktop-gui/src/specs/specs-list.jsx +++ b/packages/desktop-gui/src/specs/specs-list.jsx @@ -6,17 +6,15 @@ import Loader from 'react-loader' import ipc from '../lib/ipc' import projectsApi from '../projects/projects-api' -import specsStore from './specs-store' +import specsStore, { allSpecsSpec } from './specs-store' @observer -class Specs extends Component { +class SpecsList extends Component { render () { if (specsStore.isLoading) return if (!specsStore.filter && !specsStore.specs.length) return this._empty() - const allSpecsSpec = specsStore.getAllSpecsSpec() - return (
@@ -36,8 +34,8 @@ class Specs extends Component { />
- - {' '} + + {' '} {allSpecsSpec.displayName} @@ -63,27 +61,15 @@ class Specs extends Component { } _specItem (spec) { - if (spec.hasChildren()) { - return this._folderContent(spec) - } else { - return this._specContent(spec) - } + return spec.hasChildren ? this._folderContent(spec) : this._specContent(spec) } _allSpecsIcon (allSpecsChosen) { - if (allSpecsChosen) { - return 'fa-dot-circle-o green' - } else { - return 'fa-play' - } + return allSpecsChosen ? 'fa-dot-circle-o green' : 'fa-play' } _specIcon (isChosen) { - if (isChosen) { - return 'fa-dot-circle-o green' - } else { - return 'fa-file-code-o' - } + return isChosen ? 'fa-dot-circle-o green' : 'fa-file-code-o' } _clearFilter = () => { @@ -142,10 +128,10 @@ class Specs extends Component { _specContent (spec) { return (
  • - +
    - + {spec.displayName}
    @@ -186,4 +172,4 @@ class Specs extends Component { } } -export default Specs +export default SpecsList diff --git a/packages/desktop-gui/src/specs/specs-store.js b/packages/desktop-gui/src/specs/specs-store.js index 3bdc0b3e8c..b096a47175 100644 --- a/packages/desktop-gui/src/specs/specs-store.js +++ b/packages/desktop-gui/src/specs/specs-store.js @@ -1,57 +1,63 @@ import _ from 'lodash' import { action, computed, observable } from 'mobx' +import path from 'path' import localData from '../lib/local-data' import Spec from './spec-model' +import Folder from './folder-model' -const ALL_SPECS = '__all' +const pathSeparatorRe = /[\\\/]/g +const extRegex = /.*\.\w+$/ +const isFile = (maybeFile) => extRegex.test(maybeFile) + +export const allSpecsSpec = new Spec({ + name: 'All Specs', + absolute: '__all', + relative: '__all', + displayName: 'Run all specs', +}) + +const formRelativePath = (spec) => { + return spec === allSpecsSpec ? spec.relative : path.join(spec.type, spec.name) +} + +const pathsEqual = (path1, path2) => { + if (!path1 || !path2) return false + + return path1.replace(pathSeparatorRe, '') === path2.replace(pathSeparatorRe, '') +} export class SpecsStore { - @observable _specs = [] - @observable error = null + @observable _files = [] + @observable chosenSpecPath + @observable error @observable isLoading = false - @observable filter = null - - constructor () { - this.models = [] - - this.allSpecsSpec = new Spec({ - name: null, - path: ALL_SPECS, - displayName: 'Run all specs', - obj: { - name: 'All Specs', - relative: null, - absolute: null, - }, - }) - } + @observable filter @computed get specs () { - return this._tree(this._specs) + return this._tree(this._files) } @action loading (bool) { this.isLoading = bool } - @action setSpecs (specs) { - this._specs = specs + @action setSpecs (specsByType) { + this._files = _.flatten(_.map(specsByType, (specs, type) => { + return _.map(specs, (spec) => { + return _.extend({}, spec, { type }) + }) + })) this.isLoading = false } @action setChosenSpec (spec) { - // set all the models to false - _ - .chain(this.models) - .concat(this.allSpecsSpec) - .invokeMap('setChosen', false) - .value() + this.chosenSpecPath = spec ? formRelativePath(spec) : null + } - if (spec) { - spec.setChosen(true) - } + @action setChosenSpecByRelativePath (relativePath) { + this.chosenSpecPath = relativePath } @action setExpandSpecFolder (spec) { @@ -70,85 +76,45 @@ export class SpecsStore { this.filter = null } - setChosenSpecByRelativePath (relativePath) { - // TODO: currently this will always find nothing - // because this data is sent from the driver when - // a spec first opens. it passes the normalized url - // which will no longer match any spec. we need to - // change the logic to do this. it's barely worth it though. - const found = this.findSpecModelByPath(relativePath) - - if (found) { - this.setChosenSpec(found) - } + isChosen (spec) { + return pathsEqual(this.chosenSpecPath, formRelativePath(spec)) } - findOrCreateSpec (file, segment) { - const spec = new Spec({ - obj: file, // store the original obj - name: file.name, - path: file.relative, - displayName: segment, - }) - - const found = this.findSpecModelByPath(file.relative) - - if (found) { - spec.merge(found) - } - - return spec - } - - findSpecModelByPath (path) { - return _.find(this.models, { path }) - } - - getAllSpecsSpec () { - return this.allSpecsSpec - } - - _tree (specsByType) { - let specs = _.flatten(_.map(specsByType, (specs, type) => { - return _.map(specs, (spec) => { - // add type (unit, integration, etc) to beginning - // and change \\ to / for Windows - return _.extend({}, spec, { - name: `${type}/${spec.name.replace(/\\/g, '/')}`, - }) - }) - })) - + _tree (files) { if (this.filter) { - specs = _.filter(specs, (spec) => { + files = _.filter(files, (spec) => { return spec.name.toLowerCase().includes(this.filter.toLowerCase()) }) } - const specModels = [] + const tree = _.reduce(files, (root, file) => { + const segments = [file.type].concat(file.name.split(pathSeparatorRe)) + const segmentsPassed = [] - const tree = _.reduce(specs, (root, file) => { let placeholder = root - const segments = file.name.split('/') - _.each(segments, (segment) => { - let spec = _.find(placeholder, { displayName: segment }) - if (!spec) { - spec = this.findOrCreateSpec(file, segment) + segmentsPassed.push(segment) + const currentPath = path.join(...segmentsPassed) + const isCurrentAFile = isFile(currentPath) + const props = { path: currentPath, displayName: segment } - specModels.push(spec) - placeholder.push(spec) + let existing = _.find(placeholder, (file) => pathsEqual(file.path, currentPath)) + + if (!existing) { + existing = isCurrentAFile ? new Spec(_.extend(file, props)) : new Folder(props) + + placeholder.push(existing) } - placeholder = spec.children + if (!isCurrentAFile) { + placeholder = existing.children + } }) return root }, []) - this.models = specModels - return tree } }