Fix screenshot paths differing between interactive and run modes (#2103)

* 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

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)

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(() => {

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
+  }
+}

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')
   }
 }

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 <Loader color='#888' scale={0.5}/>
 
     if (!specsStore.filter && !specsStore.specs.length) return this._empty()
 
-    const allSpecsSpec = specsStore.getAllSpecsSpec()
-
     return (
       <div id='tests-list-page'>
         <header>
@@ -36,8 +34,8 @@ class Specs extends Component {
             />
             <a className='clear-filter fa fa-times' onClick={this._clearFilter} />
           </div>
-          <a onClick={this._selectSpec.bind(this, allSpecsSpec)} className={cs('all-tests btn btn-default', { active: allSpecsSpec.isChosen })}>
-            <i className={`fa fa-fw ${this._allSpecsIcon(allSpecsSpec.isChosen)}`}></i>{' '}
+          <a onClick={this._selectSpec.bind(this, allSpecsSpec)} className={cs('all-tests btn btn-default', { active: specsStore.isChosen(allSpecsSpec) })}>
+            <i className={`fa fa-fw ${this._allSpecsIcon(specsStore.isChosen(allSpecsSpec))}`}></i>{' '}
             {allSpecsSpec.displayName}
           </a>
         </header>
@@ -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 (
       <li key={spec.path} className='file'>
-        <a href='#' onClick={this._selectSpec.bind(this, spec)} className={cs({ active: spec.isChosen })}>
+        <a href='#' onClick={this._selectSpec.bind(this, spec)} className={cs({ active: specsStore.isChosen(spec) })}>
           <div>
             <div>
-              <i className={`fa fa-fw ${this._specIcon(spec.isChosen)}`}></i>
+              <i className={`fa fa-fw ${this._specIcon(specsStore.isChosen(spec))}`}></i>
               {spec.displayName}
             </div>
           </div>
@@ -186,4 +172,4 @@ class Specs extends Component {
   }
 }
 
-export default Specs
+export default SpecsList

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
   }
 }