Implement new logic for /wda/deactivateApp endpoint #381
Conversation
lib/commands/general.js
Outdated
| if (_.isUndefined(duration)) { | ||
| // TODO: Replace the block after deprecated stuff is removed | ||
| // seconds = Number.MAX_SAFE_INTEGER; | ||
| log.warn(`commands.background: Application under test will never be restored in the future if no duration is provided.\n |
There was a problem hiding this comment.
A template that spans two lines will keep the formatting. So this will have two newlines in it. Also, it will get printed weirdly in our logs, since the prefix will be skipped. I'd rather see this on a single line.
There was a problem hiding this comment.
Also this message is a bit confusing. It makes it seem like the application will not be restored, not that the behavior will change at some point in time.
There was a problem hiding this comment.
yeah, one single line or two log.warn calls, one per line
There was a problem hiding this comment.
Also also the commands.background part seems geared toward developers of Appium, whereas it is really for end users.
lib/commands/general.js
Outdated
| if (_.isUndefined(duration)) { | ||
| // TODO: Replace the block after deprecated stuff is removed | ||
| // seconds = Number.MAX_SAFE_INTEGER; | ||
| log.warn(`commands.background: Application under test will never be restored in the future if no duration is provided.\n |
There was a problem hiding this comment.
yeah, one single line or two log.warn calls, one per line
lib/commands/general.js
Outdated
| if (_.isUndefined(duration)) { | ||
| // TODO: Replace the block after deprecated stuff is removed | ||
| // seconds = Number.MAX_SAFE_INTEGER; | ||
| log.warn(`commands.background: Application under test will never be restored in the future if no duration is provided.\n |
There was a problem hiding this comment.
Also this message is a bit confusing. It makes it seem like the application will not be restored, not that the behavior will change at some point in time.
lib/commands/general.js
Outdated
| if (_.isUndefined(duration)) { | ||
| // TODO: Replace the block after deprecated stuff is removed | ||
| // seconds = Number.MAX_SAFE_INTEGER; | ||
| log.warn(`commands.background: Application under test will never be restored in the future if no duration is provided.\n |
There was a problem hiding this comment.
Also also the commands.background part seems geared toward developers of Appium, whereas it is really for end users.
lib/commands/general.js
Outdated
| if (duration >= 0) { | ||
| seconds = duration; | ||
| } else { | ||
| seconds = Number.MAX_SAFE_INTEGER; |
Yep, I've almost forgotten about one important thing. Fixed now. Friday... |
lib/commands/general.js
Outdated
| // seconds = Number.MAX_SAFE_INTEGER; | ||
| log.warn('commands.background: Application under test will never be restored in the future if no duration is provided. ' + | ||
| 'See https://github.com/appium/appium/issues/7741'); | ||
| seconds = null; |
There was a problem hiding this comment.
I feel uncomfortable with all the over-determination of seconds here. I'd rather use this logic to build up the endpoint and params, and then act on that below.
There was a problem hiding this comment.
agree. Changed the logic
As per appium/appium#7741
Please double check whether the flow is correct and matches to what was discussed in that thread.