search

thaliproject / CI

CI project for testing mobile devices
MIT License
2 stars 3 forks source link

preparation to get rid of jxcore specific utils #88

Open larryonoff opened 7 years ago

larryonoff commented 7 years ago

The most breaking changes are in tools/android/* and tasker/android.js. I general tasker/android.js was rewritten into promises.

This change is Reviewable

larryonoff commented 7 years ago

@yaronyg @andrew-aladev @chapko please review only file logger.js

larryonoff commented 7 years ago

I think it's to to start reviewing this PR.

chapko commented 7 years ago

Reviewed 9 of 15 files at r1, 6 of 9 files at r2. Review status: 15 of 19 files reviewed at latest revision, 8 unresolved discussions.

logger.js, line 12 at r2 (raw file):


const util = require('util');
const format = util.format;
const { format } = require('util');

logger.js, line 14 at r2 (raw file):

const format = util.format;

function Logger(options) {

Since you're using ES next, you can rewrite it with class syntax

logger.js, line 23 at r2 (raw file):

  const filePath = this._filePath;
  if (filePath) {
    const message = format.apply(null, messages) + '\n';

Use spread operator:

format(...messages)

logger.js, line 35 at r2 (raw file):

  const now = new Date().toISOString()
    .replace(/T/, ' ')
    .replace(/.[^.]+$/, '');

I suggest something like:

new Date().toISOString().replace(/[TZ]/, ' ').trim();

Milliseconds are important in the logs.

logger.js, line 42 at r2 (raw file):


Logger.prototype.error = function() {
  this._log(console.error, chalk.red, Array.from(arguments));

You can use rest syntax here:

error (...messages) {
  this._log(console.error, chalk.red, messages);
}

builder/virtual.js, line 332 at r2 (raw file):

        if (job.target != 'all') {
          if (job.target == 'ios') {
            execSync('cd ' + __dirname + '; rm -rf ' + prPath + '/build_android');

It reminds me of little bobby tables :)

internal/tester.js, line 10 at r2 (raw file):

var db = require('./../db_actions');
var fs = require('fs');
var git = require('./../hook/git_actions');

'./' is just extraneous here, require('../hook/git_actions') should be enough.

tasker/run_.sh, line 11 at r2 (raw file):


OUTPUT() {
  echo -e "${RED_COLOR}$BASH_COMMAND CI FAILED - sign_ios.sh failure${NORMAL_COLOR}"

how "sing_ios.sh" is related to this script?

Comments from Reviewable

larryonoff commented 7 years ago

Review status: 11 of 18 files reviewed at latest revision, 8 unresolved discussions.

logger.js, line 12 at r2 (raw file):

Previously, chapko (Eugene Chapko) wrote…
``` const { format } = require('util'); ```

Done.

logger.js, line 23 at r2 (raw file):

Previously, chapko (Eugene Chapko) wrote…
Use spread operator: ```JavaScript format(...messages) ```

Done.

logger.js, line 35 at r2 (raw file):

Previously, chapko (Eugene Chapko) wrote…
I suggest something like: ```JavaScript new Date().toISOString().replace(/[TZ]/, ' ').trim(); ``` Milliseconds are important in the logs.

Done.

internal/tester.js, line 10 at r2 (raw file):

Previously, chapko (Eugene Chapko) wrote…
`'./'` is just extraneous here, `require('../hook/git_actions')` should be enough.

Done.

tasker/run_.sh, line 11 at r2 (raw file):

Previously, chapko (Eugene Chapko) wrote…
how "sing_ios.sh" is related to this script?

Done.

Comments from Reviewable

jareksl commented 7 years ago

Review status: 11 of 18 files reviewed at latest revision, 10 unresolved discussions.

db_actions.js, line 1 at r4 (raw file):

//  Copyright (C) Microsoft. All rights reserved.

As you're fixing issues in this file it would be good to add missing ';' in line 22,line 30 and line 39

db_actions.js, line 9 at r4 (raw file):


var fs = require('fs');
var local = new loki('config.json');

Shouldn't we first declare loki with require and later instantiate it?

Comments from Reviewable

jareksl commented 7 years ago

db_actions.js, line 1 at r4 (raw file):

Previously, jareksl (Jarosław Słota) wrote…
As you're fixing issues in this file it would be good to add missing ';' in line 22,line 30 and line 39

Please also remove two not used functions find() and remove() lines 66 and 82

Comments from Reviewable

yaronyg commented 7 years ago
:lgtm:

Reviewed 8 of 15 files at r1, 7 of 9 files at r2, 3 of 3 files at r4. Review status: all files reviewed at latest revision, 12 unresolved discussions.

tasker/android.js, line 33 at r4 (raw file):


  return adb.installApp(apkPath, appId, deviceId)
    .then(() => adb.listPackages(deviceId))

NB: Since you are running this in real Node why not use the await syntax? It's SOOO much nicer!

tasker/android.js, line 101 at r4 (raw file):

}

// TODO: see original implementation

NB: What original implementation?

Comments from Reviewable

larryonoff commented 7 years ago

Review status: 16 of 18 files reviewed at latest revision, 12 unresolved discussions.

db_actions.js, line 1 at r4 (raw file):

Previously, jareksl (Jarosław Słota) wrote…
Please also remove two not used functions find() and remove() lines 66 and 82

Done.

db_actions.js, line 9 at r4 (raw file):

Previously, jareksl (Jarosław Słota) wrote…
Shouldn't we first declare loki with require and later instantiate it?

Done.

tasker/android.js, line 33 at r4 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…
NB: Since you are running this in real Node why not use the await syntax? It's SOOO much nicer!

I absolutely agree. I was initially using Promises when I didn't expect that I would use node.

tasker/android.js, line 101 at r4 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…
NB: What original implementation?

This's unnecessary TODO I forgot to remove.

Comments from Reviewable

czyzm commented 7 years ago

Reviewed 1 of 9 files at r2. Review status: 16 of 18 files reviewed at latest revision, 12 unresolved discussions.

Comments from Reviewable

czyzm commented 7 years ago

Review status: 16 of 18 files reviewed at latest revision, 13 unresolved discussions.

tasker/android.js, line 216 at r5 (raw file):

    .then((devices) => {
      if (serverDir && serverDir.length) {
        return exec(`curl 192.168.1.150:8060/droid=${devices.length}`)

We need to make sure that before the test app is started the coordination server is running (in case it is needed). You can check the following issue for details: https://github.com/thaliproject/CI/issues/97.

Branch master_97_rework contains a temporary solution which introduces a timeout. We need to fix this here. In worst case it could be timeout as well but the best option would be to wait till coordination server is started. The server starts after this call but it takes some time as it includes npm install. We need some mechanism in ____.js so that we can ask the server if the coordination server has been started. Then we may continue.

Comments from Reviewable

larryonoff commented 7 years ago

Review status: 16 of 18 files reviewed at latest revision, 13 unresolved discussions.

tasker/android.js, line 216 at r5 (raw file):

Previously, czyzm wrote…
We need to make sure that before the test app is started the coordination server is running (in case it is needed). You can check the following issue for details: https://github.com/thaliproject/CI/issues/97. Branch master_97_rework contains a temporary solution which introduces a timeout. We need to fix this here. In worst case it could be timeout as well but the best option would be to wait till coordination server is started. The server starts after this call but it takes some time as it includes npm install. We need some mechanism in ____.js so that we can ask the server if the coordination server has been started. Then we may continue.

what if we just add ping with retries and concrete timeout ?

Comments from Reviewable

jareksl commented 7 years ago

Reviewed 1 of 15 files at r1, 2 of 9 files at r2, 1 of 2 files at r5. Review status: 17 of 18 files reviewed at latest revision, 23 unresolved discussions.

db_actions.js, line 23 at r5 (raw file):

  console.log('Creating config.json');
  local.addCollection('config');
  // local.getCollection('config').insert({name: "GithubUser", username: "obastemur", password: ""})

NB: Leaving a commented code is usually not good, we have version control to handle such cases. I'd prefer to remove it from here.

builder/virtual.js, line 330 at r5 (raw file):

        }

        if (job.target != 'all') {

The == and != operators do type coercion and should not be used. Please fix this issue in all places in this file

hook/git_actions.js, line 12 at r5 (raw file):

var reporting = require('../reporting/report');
var tester = require('./../internal/tester');
var virtual = require('./../builder/virtual')

Please add missing semicolon.

hook/git_actions.js, line 154 at r5 (raw file):

      createIssue(user, repo, "mobile_test.json is missing", "Skipped testing branch `" + branch + "`. There was no mobile_test.json file");
      logger.info("skipped testing branch", branch, "on", user + "/" + repo, "(no mobile_test.json found)");
      return;

The return statement is not needed here. You can also leave it and get rid of succeeding "else" leaving the content, which would simplify the code a bit by removing one intent.

hook/git_request.js, line 25 at r5 (raw file):


  if (json.pull_request) { // pull request
    if (json.action != 'opened' &&

The != operators do type coercion and should not be used. Use !== instead.

hook/git_request.js, line 60 at r5 (raw file):

    }

    if (!repo_name || repo_name.indexOf('thaliproject/') != 0 &&

We can get rid of the second part of the if statement: repo_name.indexOf('thaliproject/') != 0 && repo_name.indexOf('obastemur/') != 0 Most probably it was used by Iz for testing purposes

hook/git_request.js, line 62 at r5 (raw file):

    if (!repo_name || repo_name.indexOf('thaliproject/') != 0 &&
      repo_name.indexOf('obastemur/') != 0) {
      if (pr && pr.state && pr.state != 'open') {

Why not simply check for negative and log it? like: if (!pr || !pr.state || pr.state === 'open') { logger.error('BAD REQUEST from repo', repo_name); }

Additionally this statement could be combined with the one from the line 54

hook/git_request.js, line 78 at r5 (raw file):

  } else if (json.hook && (typeof json.hook_id !== 'undefined')) { // new web hook
    var hook_id = json.hook_id;
    var repo_name = json.repository.full_name;

Variables repo_name, branch and user are declared again here. Better would be to declare all variables at the beginning of the file. Vars don't have block scope, so defining variables in blocks can be confusing.

internal/tester.js, line 223 at r5 (raw file):

        if (fs.existsSync(name + "/result_.json")) {
          try {
            var res = JSON.parse(fs.readFileSync(name + "/result_.json") + "");

Variable res is declared again

internal/tester.js, line 226 at r5 (raw file):

            for(var o in res) {
              if (res.hasOwnProperty(o)) {
                var str = res[o];

Variables str and fname are declared again. The same for variable o. They should be declared once at the beginning of the file.

Comments from Reviewable

larryonoff commented 7 years ago

@jareksl Please note that CI has a lot of crappy code, which was introduced before we took the project. This's definitely MUST be fixed. But I think that this should not be the focus of this PR review. At least because this PR will last forever and fixing warnings / bad code isn't the goal of this PR.

I think that code cleanup should be another PR.

Review status: 17 of 18 files reviewed at latest revision, 23 unresolved discussions.

Comments from Reviewable

czyzm commented 7 years ago

Review status: 17 of 18 files reviewed at latest revision, 23 unresolved discussions.

tasker/android.js, line 216 at r5 (raw file):

Previously, larryonoff (larryonoff) wrote…
what if we just add `ping` with retries and concrete timeout ?

No need to handle it. Fixed by https://github.com/thaliproject/Thali_CordovaPlugin/pull/1712

Comments from Reviewable

jareksl commented 7 years ago

I agree, I have just reported issues to not forget about them, I'll create separate issue to refactor the CI code

Review status: 17 of 18 files reviewed at latest revision, 23 unresolved discussions.

Comments from Reviewable

larryonoff commented 7 years ago

Review status: 17 of 18 files reviewed at latest revision, 23 unresolved discussions.

tasker/android.js, line 216 at r5 (raw file):

Previously, czyzm wrote…
No need to handle it. Fixed by https://github.com/thaliproject/Thali_CordovaPlugin/pull/1712

this's very good!

Comments from Reviewable

jareksl commented 7 years ago

The issue #99 is meant to be used for code cleanup. :lgtm:

Reviewed 6 of 15 files at r1, 4 of 9 files at r2, 3 of 3 files at r4, 1 of 2 files at r5. Review status: all files reviewed at latest revision, 24 unresolved discussions.

tools/android/adb.js, line 15 at r5 (raw file):

    this._logger = logger;
  }
};

NB: not needed semicolon

Comments from Reviewable

czyzm commented 7 years ago

We need to adapt CI infrastructure for those changes. The main change is installing node on RaspberryPI modules. This needs to wait for now.