From 2feca8e65c56d609706f39f4a42a148202a0eea7 Mon Sep 17 00:00:00 2001 From: Andrew Smith Date: Wed, 4 Dec 2019 09:57:07 -0500 Subject: [PATCH 1/7] env config settings is displayed correctly Previously was being dropped as the value of `env` was slightly different from the rest of the resolved config. It did not have an explicit `from`/`value` combo. NormalizeWithoutMeta strips `from`/`value` to simply the value. This is not explictly done for the env key's values. --- .../cypress/integration/settings_spec.js | 50 +++++++++++ .../src/settings/configuration.jsx | 88 +++++++++++++------ 2 files changed, 109 insertions(+), 29 deletions(-) diff --git a/packages/desktop-gui/cypress/integration/settings_spec.js b/packages/desktop-gui/cypress/integration/settings_spec.js index f3755707aadd..3c39fcc5efd1 100644 --- a/packages/desktop-gui/cypress/integration/settings_spec.js +++ b/packages/desktop-gui/cypress/integration/settings_spec.js @@ -1,4 +1,5 @@ const { _ } = Cypress +const { each, flow, get, isString, join, map, sortBy, toPairs } = require('lodash/fp') describe('Settings', () => { beforeEach(function () { @@ -181,6 +182,55 @@ describe('Settings', () => { expect(this.ipc.externalOpen).to.be.calledWith('https://on.cypress.io/guides/configuration') }) }) + + it('displays env settings', () => { + cy.get('@config').then(({ resolved }) => { + const getEnvKeys = flow([ + get('env'), + toPairs, + map(([key]) => key), + sortBy(get('')), + ]) + + cy.contains('.line', 'env').contains(flow([getEnvKeys, join(', ')])(resolved)) + cy.contains('.line', 'env').click() + const assertKeyExists = each((key) => cy.contains('.line', key)) + const assertKeyValuesExists = flow([ + map((key) => { + return flow([ + get(['env', key, 'value']), + (v) => { + if (isString(v)) { + return `"${v}"` + } + + return v + }, + ])(resolved) + }), + each((v) => { + cy.contains('.key-value-pair-value', v) + }), + ]) + + const assertFromTooltipsExist = flow([ + map((key) => { + return [key, + flow([ + get(['env', key, 'from']), + (from) => `.${from}`, + ])(resolved)] + }), + each(([key, fromTooltipClassName]) => { + cy.contains(key).parents('.line').first().find(fromTooltipClassName) + }), + ]) + + flow([getEnvKeys, assertKeyExists])(resolved) + flow([getEnvKeys, assertKeyValuesExists])(resolved) + flow([getEnvKeys, assertFromTooltipsExist])(resolved) + }) + }) }) describe('when project id panel is opened', () => { diff --git a/packages/desktop-gui/src/settings/configuration.jsx b/packages/desktop-gui/src/settings/configuration.jsx index ef05d286675c..bdca7694b7eb 100644 --- a/packages/desktop-gui/src/settings/configuration.jsx +++ b/packages/desktop-gui/src/settings/configuration.jsx @@ -1,38 +1,50 @@ import _ from 'lodash' +import { defaultTo, get, flow, join, map, reduce, take, toPairs, toPath } from 'lodash/fp' import cn from 'classnames' import { observer } from 'mobx-react' import React from 'react' import Tooltip from '@cypress/react-tooltip' import { ObjectInspector, ObjectName } from 'react-inspector' - import { configFileFormatted } from '../lib/config-file-formatted' import ipc from '../lib/ipc' -const formatData = (data) => { - if (Array.isArray(data)) { - return _.map(data, (v) => { - if (_.isObject(v) && (v.name || v.displayName)) { - return _.defaultTo(v.displayName, v.name) - } - - return String(v) - }).join(', ') +const joinWithCommas = join(', ') +const objToString = (v) => flow([ + defaultTo(v.name), + defaultTo(_.isObject(v) ? joinWithCommas(Object.keys(v)) : undefined), + defaultTo(String(v)), +])(v.displayName) + +const formatValue = (value) => { + if (Array.isArray(value)) { + return flow([ + map(objToString), + joinWithCommas, + ])(value) } - if (_.isObject(data)) { - return _.defaultTo(_.defaultTo(data.displayName, data.name), String(Object.keys(data).join(', '))) + if (_.isObject(value)) { + return objToString(value) } const excludedFromQuotations = ['null', 'undefined'] - if (_.isString(data) && !excludedFromQuotations.includes(data)) { - return `"${data}"` + if (_.isString(value) && !excludedFromQuotations.includes(value)) { + return `"${value}"` } - return String(data) + return String(value) } + +const normalizeWithoutMeta = flow([ + toPairs, + reduce((acc, [key, value]) => _.merge({}, acc, { + [key]: value.value, + }), {}), +]) + const ObjectLabel = ({ name, data, expanded, from, isNonenumerable }) => { - const formattedData = formatData(data) + const formattedData = formatValue(data) return ( @@ -40,11 +52,18 @@ const ObjectLabel = ({ name, data, expanded, from, isNonenumerable }) => { : {!expanded && ( <> - + {from && ( + + + {formattedData} + + + )} + {!from && ( {formattedData} - + )} )} {expanded && Array.isArray(data) && ( @@ -58,25 +77,36 @@ ObjectLabel.defaultProps = { data: 'undefined', } -const createComputeFromValue = (obj) => { - return (name, path) => { - const pathParts = path.split('.') - const pathDepth = pathParts.length +const computeFromValue = (obj, name, path) => { + const normalizedPath = path.replace('$.', '').replace(name, `['${name}']`) + const getValueForPath = flow([ + toPath, + _.partialRight(get, obj), + ]) + + let value = getValueForPath(normalizedPath) - const rootKey = pathDepth <= 2 ? name : pathParts[1] + if (!value) { + const onlyFirstKeyInPath = flow([toPath, take(1)]) - return obj[rootKey] ? obj[rootKey].from : undefined + value = getValueForPath(onlyFirstKeyInPath(normalizedPath)) } + + if (!value) { + return undefined + } + + return value.from ? value.from : undefined } const ConfigDisplay = ({ data: obj }) => { - const computeFromValue = createComputeFromValue(obj) + const getFromValue = _.partial(computeFromValue, obj) const renderNode = ({ depth, name, data, isNonenumerable, expanded, path }) => { if (depth === 0) { return null } - const from = computeFromValue(name, path) + const from = getFromValue(name, path) return ( { ) } - const data = _.reduce(obj, (acc, value, key) => Object.assign(acc, { - [key]: value.value, - }), {}) + const data = normalizeWithoutMeta(obj) + + data.env = normalizeWithoutMeta(obj.env) return (
From 86b6b746cceb6aefac6d0ed1343918758927557c Mon Sep 17 00:00:00 2001 From: Andrew Smith Date: Fri, 6 Dec 2019 14:05:11 -0500 Subject: [PATCH 2/7] properly handle when env vars are not set; should show as null --- .../cypress/integration/settings_spec.js | 25 ++++++++++++++++++- .../src/settings/configuration.jsx | 12 +++++++-- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/packages/desktop-gui/cypress/integration/settings_spec.js b/packages/desktop-gui/cypress/integration/settings_spec.js index 3c39fcc5efd1..19cc35121bc4 100644 --- a/packages/desktop-gui/cypress/integration/settings_spec.js +++ b/packages/desktop-gui/cypress/integration/settings_spec.js @@ -1,5 +1,5 @@ const { _ } = Cypress -const { each, flow, get, isString, join, map, sortBy, toPairs } = require('lodash/fp') +const { each, flow, get, isString, join, map, merge, set, sortBy, toPairs } = require('lodash/fp') describe('Settings', () => { beforeEach(function () { @@ -183,6 +183,29 @@ describe('Settings', () => { }) }) + it('displays null when no env settings are found', function () { + const updateEnvConfig = (v) => { + return flow([ + merge(this.config), + set('resolved.env', v), + ])({}) + } + + const nullEnvConfig = updateEnvConfig(null) + + this.ipc.openProject.onCall(1).resolves(nullEnvConfig) + this.ipc.onConfigChanged.yield() + + cy.contains('.line', 'env:null') + + const emptyEnvConfig = updateEnvConfig({}) + + this.ipc.openProject.onCall(2).resolves(emptyEnvConfig) + this.ipc.onConfigChanged.yield() + + cy.contains('.line', 'env:null') + }) + it('displays env settings', () => { cy.get('@config').then(({ resolved }) => { const getEnvKeys = flow([ diff --git a/packages/desktop-gui/src/settings/configuration.jsx b/packages/desktop-gui/src/settings/configuration.jsx index bdca7694b7eb..4f1cd17d1930 100644 --- a/packages/desktop-gui/src/settings/configuration.jsx +++ b/packages/desktop-gui/src/settings/configuration.jsx @@ -1,5 +1,5 @@ import _ from 'lodash' -import { defaultTo, get, flow, join, map, reduce, take, toPairs, toPath } from 'lodash/fp' +import { defaultTo, get, flow, isEmpty, join, map, reduce, take, toPairs, toPath } from 'lodash/fp' import cn from 'classnames' import { observer } from 'mobx-react' import React from 'react' @@ -37,10 +37,18 @@ const formatValue = (value) => { } const normalizeWithoutMeta = flow([ + defaultTo({}), toPairs, reduce((acc, [key, value]) => _.merge({}, acc, { - [key]: value.value, + [key]: value ? value.value : {}, }), {}), + (v) => { + if (isEmpty(v)) { + return null + } + + return v + }, ]) const ObjectLabel = ({ name, data, expanded, from, isNonenumerable }) => { From 1aca6cec424335788000359d4d906c18b9c0973e Mon Sep 17 00:00:00 2001 From: Andrew Smith Date: Fri, 6 Dec 2019 15:10:00 -0500 Subject: [PATCH 3/7] include test for `undefined` env vars --- .../cypress/integration/settings_spec.js | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/desktop-gui/cypress/integration/settings_spec.js b/packages/desktop-gui/cypress/integration/settings_spec.js index 19cc35121bc4..dc8593d1a34a 100644 --- a/packages/desktop-gui/cypress/integration/settings_spec.js +++ b/packages/desktop-gui/cypress/integration/settings_spec.js @@ -184,23 +184,24 @@ describe('Settings', () => { }) it('displays null when no env settings are found', function () { - const updateEnvConfig = (v) => { + const setConfigEnv = (config, v) => { return flow([ - merge(this.config), + merge(config), set('resolved.env', v), ])({}) } - const nullEnvConfig = updateEnvConfig(null) - - this.ipc.openProject.onCall(1).resolves(nullEnvConfig) + this.ipc.openProject.onCall(1).resolves(setConfigEnv(this.config, null)) this.ipc.onConfigChanged.yield() cy.contains('.line', 'env:null') - const emptyEnvConfig = updateEnvConfig({}) + this.ipc.openProject.onCall(2).resolves(setConfigEnv(this.config, {})) + this.ipc.onConfigChanged.yield() + + cy.contains('.line', 'env:null') - this.ipc.openProject.onCall(2).resolves(emptyEnvConfig) + this.ipc.openProject.onCall(3).resolves(setConfigEnv(this.config, undefined)) this.ipc.onConfigChanged.yield() cy.contains('.line', 'env:null') @@ -215,8 +216,6 @@ describe('Settings', () => { sortBy(get('')), ]) - cy.contains('.line', 'env').contains(flow([getEnvKeys, join(', ')])(resolved)) - cy.contains('.line', 'env').click() const assertKeyExists = each((key) => cy.contains('.line', key)) const assertKeyValuesExists = flow([ map((key) => { @@ -249,6 +248,8 @@ describe('Settings', () => { }), ]) + cy.contains('.line', 'env').contains(flow([getEnvKeys, join(', ')])(resolved)) + cy.contains('.line', 'env').click() flow([getEnvKeys, assertKeyExists])(resolved) flow([getEnvKeys, assertKeyValuesExists])(resolved) flow([getEnvKeys, assertFromTooltipsExist])(resolved) From ffa2aca702c4aa91f3581b8b2dd653e71b9cb3fb Mon Sep 17 00:00:00 2001 From: Brian Mann Date: Thu, 12 Dec 2019 13:17:42 -0500 Subject: [PATCH 4/7] correctly await yielding the onConfigChanged callbacks --- .../cypress/integration/settings_spec.js | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/packages/desktop-gui/cypress/integration/settings_spec.js b/packages/desktop-gui/cypress/integration/settings_spec.js index dc8593d1a34a..aef4640f73c4 100644 --- a/packages/desktop-gui/cypress/integration/settings_spec.js +++ b/packages/desktop-gui/cypress/integration/settings_spec.js @@ -191,20 +191,24 @@ describe('Settings', () => { ])({}) } - this.ipc.openProject.onCall(1).resolves(setConfigEnv(this.config, null)) - this.ipc.onConfigChanged.yield() - - cy.contains('.line', 'env:null') + this.ipc.openProject + .onCall(1).resolves(setConfigEnv(this.config, null)) + .onCall(3).resolves(setConfigEnv(this.config, undefined)) + .onCall(2).resolves(setConfigEnv(this.config, {})) - this.ipc.openProject.onCall(2).resolves(setConfigEnv(this.config, {})) this.ipc.onConfigChanged.yield() cy.contains('.line', 'env:null') + .then(() => { + this.ipc.onConfigChanged.yield() - this.ipc.openProject.onCall(3).resolves(setConfigEnv(this.config, undefined)) - this.ipc.onConfigChanged.yield() + cy.contains('.line', 'env:null') + .then(() => { + this.ipc.onConfigChanged.yield() - cy.contains('.line', 'env:null') + cy.contains('.line', 'env:null') + }) + }) }) it('displays env settings', () => { From 98320cf4da9ece9e4cd238e14117cf8b599c23d3 Mon Sep 17 00:00:00 2001 From: Andrew Smith Date: Thu, 12 Dec 2019 13:36:21 -0500 Subject: [PATCH 5/7] make tests pass implementation works individually for each of the three test cases. I don't like breaking them into three separate E2E tests, but I'm unsure how to orchestrate them properly otherwise. --- .../cypress/integration/settings_spec.js | 43 +++++++++++-------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/packages/desktop-gui/cypress/integration/settings_spec.js b/packages/desktop-gui/cypress/integration/settings_spec.js index aef4640f73c4..3005e1ee78e5 100644 --- a/packages/desktop-gui/cypress/integration/settings_spec.js +++ b/packages/desktop-gui/cypress/integration/settings_spec.js @@ -183,32 +183,31 @@ describe('Settings', () => { }) }) - it('displays null when no env settings are found', function () { - const setConfigEnv = (config, v) => { - return flow([ - merge(config), - set('resolved.env', v), - ])({}) - } + it('displays null when no env settings are null', function () { + this.ipc.openProject + .onCall(0).resolves(setConfigEnv(this.config, null)) + + this.ipc.onConfigChanged.yield() + cy.contains('.line', 'env:null') + }) + + it('displays null when no env settings are undefined', function () { this.ipc.openProject - .onCall(1).resolves(setConfigEnv(this.config, null)) - .onCall(3).resolves(setConfigEnv(this.config, undefined)) - .onCall(2).resolves(setConfigEnv(this.config, {})) + .onCall(0).resolves(setConfigEnv(this.config, undefined)) this.ipc.onConfigChanged.yield() cy.contains('.line', 'env:null') - .then(() => { - this.ipc.onConfigChanged.yield() + }) - cy.contains('.line', 'env:null') - .then(() => { - this.ipc.onConfigChanged.yield() + it('displays null when no env settings are empty', function () { + this.ipc.openProject + .onCall(0).resolves(setConfigEnv(this.config, {})) - cy.contains('.line', 'env:null') - }) - }) + this.ipc.onConfigChanged.yield() + + cy.contains('.line', 'env:null') }) it('displays env settings', () => { @@ -597,3 +596,11 @@ describe('Settings', () => { }) }) }) + +// -- +function setConfigEnv (config, v) { + return flow([ + merge(config), + set('resolved.env', v), + ])({}) +} From 3690eedf736732f8c3084a6d8a6e683134e565ad Mon Sep 17 00:00:00 2001 From: Andrew Smith Date: Thu, 12 Dec 2019 15:39:08 -0500 Subject: [PATCH 6/7] make tests pass; for real --- .../desktop-gui/cypress/integration/settings_spec.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/desktop-gui/cypress/integration/settings_spec.js b/packages/desktop-gui/cypress/integration/settings_spec.js index 3005e1ee78e5..352ee466530c 100644 --- a/packages/desktop-gui/cypress/integration/settings_spec.js +++ b/packages/desktop-gui/cypress/integration/settings_spec.js @@ -184,8 +184,7 @@ describe('Settings', () => { }) it('displays null when no env settings are null', function () { - this.ipc.openProject - .onCall(0).resolves(setConfigEnv(this.config, null)) + this.ipc.openProject.resolves(setConfigEnv(this.config, null)) this.ipc.onConfigChanged.yield() @@ -193,8 +192,7 @@ describe('Settings', () => { }) it('displays null when no env settings are undefined', function () { - this.ipc.openProject - .onCall(0).resolves(setConfigEnv(this.config, undefined)) + this.ipc.openProject.resolves(setConfigEnv(this.config, undefined)) this.ipc.onConfigChanged.yield() @@ -202,8 +200,7 @@ describe('Settings', () => { }) it('displays null when no env settings are empty', function () { - this.ipc.openProject - .onCall(0).resolves(setConfigEnv(this.config, {})) + this.ipc.openProject.resolves(setConfigEnv(this.config, {})) this.ipc.onConfigChanged.yield() From 5423399b27329b82b90d3091d7297ef856459193 Mon Sep 17 00:00:00 2001 From: Andrew Smith Date: Thu, 12 Dec 2019 16:31:07 -0500 Subject: [PATCH 7/7] improve tests - only a single test; no longer 3 separate tests for the same case --- .../cypress/integration/settings_spec.js | 41 ++++++++++--------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/packages/desktop-gui/cypress/integration/settings_spec.js b/packages/desktop-gui/cypress/integration/settings_spec.js index 352ee466530c..24be6dd8901e 100644 --- a/packages/desktop-gui/cypress/integration/settings_spec.js +++ b/packages/desktop-gui/cypress/integration/settings_spec.js @@ -183,28 +183,31 @@ describe('Settings', () => { }) }) - it('displays null when no env settings are null', function () { - this.ipc.openProject.resolves(setConfigEnv(this.config, null)) - - this.ipc.onConfigChanged.yield() - - cy.contains('.line', 'env:null') - }) - - it('displays null when no env settings are undefined', function () { + it('displays null when env settings are empty or not defined', function () { this.ipc.openProject.resolves(setConfigEnv(this.config, undefined)) - this.ipc.onConfigChanged.yield() - cy.contains('.line', 'env:null') - }) - - it('displays null when no env settings are empty', function () { - this.ipc.openProject.resolves(setConfigEnv(this.config, {})) - - this.ipc.onConfigChanged.yield() - - cy.contains('.line', 'env:null') + cy.contains('.line', 'env:null').then(() => { + this.ipc.openProject.resolves(this.config) + this.ipc.onConfigChanged.yield() + + cy.contains('.line', 'env:fileServerFolder') + .then(() => { + this.ipc.openProject.resolves(setConfigEnv(this.config, null)) + this.ipc.onConfigChanged.yield() + cy.contains('.line', 'env:null').then(() => { + this.ipc.openProject.resolves(this.config) + this.ipc.onConfigChanged.yield() + + cy.contains('.line', 'env:fileServerFolder') + .then(() => { + this.ipc.openProject.resolves(setConfigEnv(this.config, {})) + this.ipc.onConfigChanged.yield() + cy.contains('.line', 'env:null') + }) + }) + }) + }) }) it('displays env settings', () => {