add status to suites #20

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@fcarstens
Contributor

fcarstens commented Jul 26, 2015

This PR fixes #19. The suite status is available at globalSuite.getTotal().status. test.errors should already be there?

summary.passed++;
+ } else if (test.status === "skipped"){

This comment has been minimized.

@jzaefferer

jzaefferer Jul 27, 2015

Contributor

Missing space before curly brace. Should add eslint rule for that.

@jzaefferer

jzaefferer Jul 27, 2015

Contributor

Missing space before curly brace. Should add eslint rule for that.

This comment has been minimized.

@fcarstens

fcarstens Aug 3, 2015

Contributor

My bad, didn't run lint. There's already a rule for that. Fixed in #23

@fcarstens

fcarstens Aug 3, 2015

Contributor

My bad, didn't run lint. There's already a rule for that. Fixed in #23

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jul 27, 2015

Contributor

I'm still not happy with the getTotal() method. Can we just run this inside the constructor and add the properties to the instance? Though according to our current draft, there should be runtime, suites, tests and status.

Contributor

jzaefferer commented Jul 27, 2015

I'm still not happy with the getTotal() method. Can we just run this inside the constructor and add the properties to the instance? Though according to our current draft, there should be runtime, suites, tests and status.

@fcarstens

This comment has been minimized.

Show comment
Hide comment
@fcarstens

fcarstens Jul 30, 2015

Contributor

This is problematic if we want to pass Test and Suite objects on the start events as well. We would then need to recalculate them somehow later on...
Alternatively, we could use es5 getters:

class Suite {
  constructor(){
    this.tests = [1,2,3]; // Simple Demo
  }
  getAllTests(){
    return this.tests;
  }
  get total(){
    return this.getAllTests().length;
  }
}
Object.defineProperties(Suite.prototype, {
  toJSON: {
    value: function(){
      let ret = {};
      for(let x in this){
        ret[x] = this[x];
      }
      return ret;
    }
  },
  total: {
    enumerable: true
  }
})

var s = new Suite();
console.log(s.total, JSON.stringify(s));
// 3 {"tests":[1,2,3],"total":3}

What do you think?

Contributor

fcarstens commented Jul 30, 2015

This is problematic if we want to pass Test and Suite objects on the start events as well. We would then need to recalculate them somehow later on...
Alternatively, we could use es5 getters:

class Suite {
  constructor(){
    this.tests = [1,2,3]; // Simple Demo
  }
  getAllTests(){
    return this.tests;
  }
  get total(){
    return this.getAllTests().length;
  }
}
Object.defineProperties(Suite.prototype, {
  toJSON: {
    value: function(){
      let ret = {};
      for(let x in this){
        ret[x] = this[x];
      }
      return ret;
    }
  },
  total: {
    enumerable: true
  }
})

var s = new Suite();
console.log(s.total, JSON.stringify(s));
// 3 {"tests":[1,2,3],"total":3}

What do you think?

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jul 31, 2015

Contributor

Okay, let's give that a try.

Contributor

jzaefferer commented Jul 31, 2015

Okay, let's give that a try.

@fcarstens

This comment has been minimized.

Show comment
Hide comment
@fcarstens

fcarstens Aug 3, 2015

Contributor

I implemented this on top of the test stuff: https://github.com/fcarstens/js-reporters/tree/suite_summary
Shall I open a new PR?

Contributor

fcarstens commented Aug 3, 2015

I implemented this on top of the test stuff: https://github.com/fcarstens/js-reporters/tree/suite_summary
Shall I open a new PR?

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Aug 4, 2015

Contributor

Maybe rebase #23 into a set of reasonably atomic commits, than add this on top?

Contributor

jzaefferer commented Aug 4, 2015

Maybe rebase #23 into a set of reasonably atomic commits, than add this on top?

@fcarstens

This comment has been minimized.

Show comment
Hide comment
@fcarstens

fcarstens Aug 5, 2015

Contributor

Included in #23.

Contributor

fcarstens commented Aug 5, 2015

Included in #23.

@fcarstens fcarstens closed this Aug 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment