Skip to content

Conversation

@rtakacs
Copy link
Contributor

@rtakacs rtakacs commented Mar 14, 2017

No description provided.

Runner.prototype.run = function() {
if (this.attr.skip && (this.attr.skip.indexOf('all') >= 0 ||
this.attr.skip.indexOf(this.driver.os) >= 0)) {
if (skip = this.test['skip']) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope to declare skip variable for the function scope, or this skip, which someone might use this name on later, let him/her be very hard to seek to find where it is used.

Copy link
Contributor

@konradlipner konradlipner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I see no benefit to use json over js. Moreover I think this limits our use case:

  1. json is more restrictive, does not allow functions
  2. we have to load it in 3 lines, instead of 1 require, and the result is exactly the same in both cases - JS object with tests description.

Runner.prototype.run = function() {
if (this.attr.skip && (this.attr.skip.indexOf('all') >= 0 ||
this.attr.skip.indexOf(this.driver.os) >= 0)) {
if (skip = this.test['skip']) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. skip is undeclared, this is against coding rules
  2. assignment in if is a bad practice, please don't do this :)

@yichoi
Copy link
Contributor

yichoi commented Mar 15, 2017

The benefit of changing to JSON is for maintenance. Actually this JS file is not code but more like data. @kolipka, do you have any requirement to describe function to this file?

@konradlipner
Copy link
Contributor

@yichoi Currently no. In the future maybe - if we will support running tests on multiple platforms, we would possibly want to skip certain tests, not the whole file. For this purpose I would prefer to tests in test file decide does it need execution (based on some env values), rather than skipping whole file.

This is just a question. Every JSON is valid JS, so switching back cost nothing if needed. There is just one thing to fix in code, and whole change is fine for me.

@zherczeg
Copy link
Member

The motivation for this is sharing this data with other environments like python. This is needed for automated testing IoT.js on devices.

@rtakacs
Copy link
Contributor Author

rtakacs commented Mar 15, 2017

@kolipka, @chokobole: Thanks for your notices. As @zherczeg wrote, JSON like syntax is more general, many programming languages can deal with it easily. We have implemented an automatized test environment in Python (for stm32f4 board) and we would like the test information to be in a common format.

@rtakacs rtakacs force-pushed the testdriver_with_json branch from bed184f to 297cbd0 Compare March 16, 2017 07:30
@rtakacs
Copy link
Contributor Author

rtakacs commented Mar 16, 2017

I've updated the PR.

@konradlipner
Copy link
Contributor

@rtakacs Thanks. If we want to use it in python, advantage of json is clear :)

IoT.js-DCO-1.0-Signed-off-by: Roland Takacs rtakacs.u-szeged@partner.samsung.com
@rtakacs rtakacs force-pushed the testdriver_with_json branch from 297cbd0 to cba6530 Compare March 16, 2017 08:03
Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yichoi
Copy link
Contributor

yichoi commented Mar 16, 2017

LGTM

@yichoi yichoi merged commit 46e5b46 into jerryscript-project:master Mar 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants