Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement #602 and fix simple server #604

Merged
merged 4 commits into from
Feb 27, 2018
Merged

Implement #602 and fix simple server #604

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ad29964
Fix SimpleServer's port assigning issue.
st0012 Feb 25, 2018
31a4aba
Add integration test for server.
st0012 Feb 25, 2018
8081f0e
Fixed server response not set issue.
st0012 Feb 25, 2018
3bb3e9b
Refactor and add one more case to server's integration test.
st0012 Feb 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions vm/call_frame.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ type callFrame interface {
IsBlock() bool
IsSourceBlock() bool
IsRemoved() bool
setAsRemoved()
EP() *normalCallFrame
Locals() []*Pointer
LocalPtr() int
Expand All @@ -55,9 +56,7 @@ type goMethodCallFrame struct {
name string
}

func (cf *goMethodCallFrame) stopExecution() {
cf.isRemoved = true
}
func (cf *goMethodCallFrame) stopExecution() {}

type normalCallFrame struct {
*baseFrame
Expand All @@ -71,7 +70,6 @@ func (n *normalCallFrame) instructionsCount() int {
}

func (n *normalCallFrame) stopExecution() {
n.isRemoved = true
n.pc = n.instructionsCount()
}

Expand All @@ -91,6 +89,10 @@ func (b *baseFrame) IsRemoved() bool {
return b.isRemoved
}

func (b *baseFrame) setAsRemoved() {
b.isRemoved = true
}

func (b *baseFrame) IsSourceBlock() bool {
return b.isSourceBlock
}
Expand Down
13 changes: 10 additions & 3 deletions vm/instruction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,16 @@ var builtinActions = map[operationType]*action{
*/

if cf.IsBlock() {
t.callFrameStack.pop().stopExecution() // Remove block execution frame
t.callFrameStack.pop().stopExecution() // Remove method call frame
t.callFrameStack.pop().stopExecution() // Remove block source frame
/*
1. Remove block execution frame
2. Remove method call frame
3. Remove block source frame
*/
for i := 0; i < 3; i++ {
frame := t.callFrameStack.pop()
frame.stopExecution()
frame.setAsRemoved()
}
}
},
},
Expand Down
13 changes: 11 additions & 2 deletions vm/simple_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import (
"github.com/fatih/structs"
"github.com/goby-lang/goby/vm/classes"
"github.com/gorilla/mux"
"fmt"
"strconv"
Copy link
Contributor

Choose a reason for hiding this comment

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

also grouped with the std lib

Copy link
Member Author

Choose a reason for hiding this comment

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

@ear7h I already ran gofmt and I want to follow what it changes/allows

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but I believe gofmt preserves newlines so the two groups of imports are formatted independently of each other.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you insist I can change this time, but I don't see the point to group them all manually in every future PRs

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, makes sense

)

type request struct {
Expand Down Expand Up @@ -71,12 +73,19 @@ func builtinSimpleServerInstanceMethods() []*BuiltinMethodObject {
var serveStatic bool
server := receiver.(*RObject)

portVar, ok := server.InstanceVariables.get("@port")
portVar, ok := server.instanceVariableGet("@port")

if !ok {
port = "8080"
} else {
port = portVar.(*StringObject).value
switch p := portVar.(type) {
case *StringObject:
port = p.value
case *IntegerObject:
port = strconv.Itoa(p.value)
default:
fmt.Printf("Unexpected type %s for port setting\n", portVar.Class().Name)
}
}

log.Println("SimpleServer start listening on port: " + port)
Expand Down
50 changes: 50 additions & 0 deletions vm/simple_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import (
"net/http/httptest"
"strings"
"testing"
"net/http"
"io/ioutil"
"time"
)

func TestServerInitialization(t *testing.T) {
Expand All @@ -28,6 +31,53 @@ func TestServerInitialization(t *testing.T) {
}
}

func TestServerSetupResponse(t *testing.T) {
tests := []struct {
input string
expectedBody string
expectedStatus int
}{
{`
require "net/simple_server"

server = Net::SimpleServer.new(4000)
server.get("/") do |req, res|
res.body = req.method + " Hello World"
res.status = 200
end
puts("foo")
server.start
`,
"GET Hello World",
200},
}

for _, tt := range tests {
go func() {
v := initTestVM()
v.testEval(t, tt.input, getFilename())
}()

time.Sleep(1 * time.Second)
Copy link
Contributor

@ear7h ear7h Feb 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
end

the 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 tests

Copy link
Member Author

Choose a reason for hiding this comment

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

@ear7h The problem is that since we're running a web server in testEval, so it won't actually end and call waitgroup.Done()

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay, I wasn't sure of the mechanics of the test vm

resp, err := http.Get("http://localhost:4000")

if err != nil {
t.Fatal(err.Error())
}

defer resp.Body.Close()
body, err := ioutil.ReadAll(resp.Body)

if string(body) != tt.expectedBody {
t.Fatalf("Expect response body to be: \n %s, got \n %s", tt.expectedBody, string(body))
}

if resp.StatusCode != tt.expectedStatus {
t.Fatalf("Expect response status to be %d, got %d", tt.expectedStatus, resp.StatusCode)
}
}
}

func TestSetupResponseDefaultValue(t *testing.T) {
reader := strings.NewReader("")
recorder := httptest.NewRecorder()
Expand Down