Implement #602 and fix simple server #604
Conversation
ghost
assigned 
Codecov Report
@@ Coverage Diff @@
## master #604 +/- ##
==========================================
+ Coverage 82.33% 82.75% +0.41%
==========================================
Files 55 55
Lines 7231 7238 +7
==========================================
+ Hits 5954 5990 +36
+ Misses 1034 1001 -33
- Partials 243 247 +4
Continue to review full report at Codecov.
|
vm/simple_server_test.go
Outdated
| v.testEval(t, tt.input, getFilename()) | ||
| }() | ||
|
|
||
| time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
This is my only concern. Would calling server.start from a thread cause execution to end in the test VM? If not, you could use a mutex.
There was a problem hiding this comment.
What do you mean? It'll make the vm inside goroutine to run the server (and hang there). This sleep is just to make sure we don't make the request before the server is started
There was a problem hiding this comment.
I metioned the wrong syncing mechanism, meant a wait group. I just think we shouldn't rely on time.sleep for thread safety. I was curious if the following snippets would work:
thread do
server.start
endthe tests function
waitgroup.Add(1)
go func() {
...
v.testEval(...) // does the vm die after this statement? or will the server still be running?
waitgroup.Done()
}
waitgroup.Wait()
// run the testsThere was a problem hiding this comment.
@ear7h The problem is that since we're running a web server in testEval, so it won't actually end and call waitgroup.Done()
There was a problem hiding this comment.
Ah okay, I wasn't sure of the mechanics of the test vm
| @@ -10,10 +10,10 @@ import ( | |||
| "path/filepath" | |||
| "unicode" | |||
|
|
|||
| "fmt" | |||
There was a problem hiding this comment.
I think this should be with the group above
vm/simple_server.go
Outdated
| "github.com/fatih/structs" | ||
| "github.com/goby-lang/goby/vm/classes" | ||
| "github.com/gorilla/mux" | ||
| "fmt" | ||
| "strconv" |
There was a problem hiding this comment.
also grouped with the std lib
There was a problem hiding this comment.
@ear7h I already ran gofmt and I want to follow what it changes/allows
There was a problem hiding this comment.
Yes, but I believe gofmt preserves newlines so the two groups of imports are formatted independently of each other.
There was a problem hiding this comment.
If you insist I can change this time, but I don't see the point to group them all manually in every future PRs
Previously because of implementing
breakI broke our nested block execution, which caused simple server's block argument unable to function normally. So this PR is to add integration test for simple server and fix the issue.