Issue #98 core acceptance tests #101
Conversation
…lso list usage invoking corcel from the command line
reaandrew
changed the title
|
|
||
| Expect(err).To(BeNil()) | ||
|
|
||
| var runningTimeToParse = strings.Replace(summary.RunningTime, "s", "", -1) |
There was a problem hiding this comment.
Seeing the repetition here, I think it might be worth introducing either a function, or better yet a custom matcher (http://onsi.github.io/gomega/#adding-your-own-matchers) so you could just do something like:
Ω.(summary.RunningTime).Should(EqualDurationFromString(5))|
|
||
| Expect(string(output)).To(ContainSubstring("Summary")) | ||
| Expect(summary.Error).To(ContainSubstring("Request body file not found: missing-file.json")) | ||
| }) | ||
|
|
||
| It("Error non-http url in the urls file causes a run time exception #21", func() { |
There was a problem hiding this comment.
I'm guessing that since the move to GH issue 21 is not the same as it was on GitLab, might be worth just removing the #21 as I think the test name describes the behaviour sufficiently.
| @@ -40,6 +40,8 @@ const ( | |||
| LogMessageVaidURLs = "Your urls in the test specification must be absolute and valid urls" | |||
| ) | |||
|
|
|||
| var () | |||
| @@ -69,6 +71,8 @@ func HandlePanic() { | |||
| } | |||
|
|
|||
| if err := recover(); err != nil { //catch | |||
| fmt.Println("ERROR") | |||
| fmt.Println() | |||
There was a problem hiding this comment.
NO - this was going to be something - good catch.
| } | ||
|
|
||
| func parseRunningTime(data string) string { | ||
| var runningTime = regexp.MustCompile(`Running Time: ([\d\w\.]+)`) |
There was a problem hiding this comment.
You could refactor this slightly to move the regexp MustCompile and FindStringSubmatch to inside the checkMatchXXX methods resulting in:
return checkMatchString(`Running Time: ([\d\w\.]+)`, data)| } | ||
|
|
||
| func checkMatchString(match []string) string { | ||
| if len(match) != 2 { |
There was a problem hiding this comment.
Why should len(match) be 2? seems a little 'magic number' to me.
There was a problem hiding this comment.
The result of the match is:
1[0]. The full string
2[1]. The 1st matching group
n[n-1] The nth matching group
I agree it is a matching number. I will assign it to a variable.
There was a problem hiding this comment.
These are deleted now as I am using your JSON output
| } | ||
| } | ||
|
|
||
| func checkMatchString(match []string) string { |
There was a problem hiding this comment.
Shouldn't all the fields on the Summary be numeric of some kind... why do a string match?
There was a problem hiding this comment.
i KNEW you would catch this one lol ;-) OK - totally agree. I deferred it - lazy me.
|
How do you mark the review comments as done? |
| @@ -27,10 +25,11 @@ func merge(source map[string]interface{}, extra map[string]interface{}) map[stri | |||
| //PlanExecutionContext encapsulates the runtime state in order to execute | |||
| //a plan | |||
| type PlanExecutionContext struct { | |||
| //Publisher telegraph.LinkedPublisher | |||
There was a problem hiding this comment.
Lets clean up the commented out code here...
| @@ -97,7 +96,12 @@ func (instance *PlanExecutionContext) workerExecuteJob(ctx context.Context, job | |||
| for _, action := range step.After { | |||
| _ = action.Execute(ctx, nil) | |||
| } | |||
| instance.Publisher.Publish(executionResult) | |||
|
|
|||
| //inproc.Lock.Lock() | |||
There was a problem hiding this comment.
Are these lines left commented out for a reason?
| @@ -189,10 +193,11 @@ type ExecutionBranch interface { | |||
|
|
|||
| //PlanExecutor ... | |||
| type PlanExecutor struct { | |||
| //Publisher telegraph.LinkedPublisher | |||
| Publisher: telegraph.NewLinkedPublisher(), | ||
| Config: config, | ||
| Bar: bar, | ||
| //Publisher: telegraph.NewLinkedPublisher(), |
|
|
||
| go func() { | ||
| for range channel { | ||
| } |
There was a problem hiding this comment.
Is this correct that we are ranging over the channel and doing nothing?
There was a problem hiding this comment.
Yeh - only so that I can write to the channel and not have to know how many messages I am going to send. I could avoid this loop and instead create a buffered channel i.e.
var channel = make(channel, numberOfMessages)
To avoid having to think of what value numberOfMessages is going to be I simply did that iteration so that the channels would drain but I am not interested in the message that is on the channel.
There was a problem hiding this comment.
Thing is though if I get the value of numberOfMessages wrong, the write operation to the channel will block. With this way it won't.
|
|
||
| var _ = Describe("GreaterThanAssertionParser", func() { | ||
|
|
||
| It("Parses", func() { |
There was a problem hiding this comment.
What about some It("fails to parse") tests
| ) | ||
|
|
||
| var _ = Describe("GreaterThanOrEqualAssertionParser", func() { | ||
| It("Parses", func() { |
There was a problem hiding this comment.
What about some It("fails to parse") tests
| ) | ||
|
|
||
| var _ = Describe("LessThanAssertionParser", func() { | ||
| It("Parses", func() { |
There was a problem hiding this comment.
What about some It("fails to parse") tests
| ) | ||
|
|
||
| var _ = Describe("LessThanOrEqualAssertionParser", func() { | ||
| It("Parses", func() { |
There was a problem hiding this comment.
What about some It("fails to parse") tests
| ) | ||
|
|
||
| var _ = Describe("NotEmptyAssertionParser", func() { | ||
| It("Parses", func() { |
There was a problem hiding this comment.
What about some It("fails to parse") tests
Closes #98
Closes #102
Closes #99