damccorm commented on code in PR #23032: URL: https://github.com/apache/beam/pull/23032#discussion_r965939872
########## learning/tour-of-beam/backend/integration_tests/client.go: ########## @@ -0,0 +1,68 @@ +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "encoding/json" + "io" + "net/http" + "os" +) + +func SdkList(url string) (sdkListResponse, error) { + var result sdkListResponse + err := Get(&result, url, nil) + return result, err +} + +func GetContentTree(url, sdk string) (ContentTree, error) { + var result ContentTree + err := Get(&result, url, map[string]string{"sdk": sdk}) + return result, err +} + +func GetUnitContent(url, sdk, unitId string) (Unit, error) { + var result Unit + err := Get(&result, url, map[string]string{"sdk": sdk, "unitId": unitId}) + return result, err +} + +// Generic HTTP call wrapper +// params: +// * dst: response struct pointer +// * url: request url +// * query_params: url query params, as a map (we don't use multiple-valued params) +func Get(dst interface{}, url string, queryParams map[string]string) error { + + req, err := http.NewRequest(http.MethodGet, url, nil) Review Comment: ```suggestion func Get(dst interface{}, url string, queryParams map[string]string) error { req, err := http.NewRequest(http.MethodGet, url, nil) ``` Nit - spacing ########## learning/tour-of-beam/backend/integration_tests/function_test.go: ########## @@ -0,0 +1,149 @@ +//go:build integration +// +build integration + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "encoding/json" + "log" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" +) + +const ( + PORT_SDK_LIST = "PORT_SDK_LIST" + PORT_GET_CONTENT_TREE = "PORT_GET_CONTENT_TREE" + PORT_GET_UNIT_CONTENT = "PORT_GET_UNIT_CONTENT" +) + +// scenarios: +// + Get SDK list +// + Get content tree for existing SDK +// - Get content tree for non-existing SDK: 404 Not Found +// - Get unit content for existing SDK, existing unitId +// - Get unit content for non-existing SDK/unitId: 404 Not Found +// TODO: +// - Get content tree for a registered user +// - Get unit content for a registered user +// - Save user code/progress for a registered user +// - (negative) Save user code/progress w/o user token/bad token +// - (negative) Save user code/progress for non-existing SDK/unitId: 404 Not Found + +func loadJson(path string, dst interface{}) error { + fh, err := os.Open(path) + if err != nil { + return err + } + return json.NewDecoder(fh).Decode(dst) +} + +func TestSdkList(t *testing.T) { + port := os.Getenv(PORT_SDK_LIST) + if port == "" { + log.Fatal(PORT_SDK_LIST, "env not set") Review Comment: ```suggestion t.Fatal(PORT_SDK_LIST, "env not set") ``` ########## learning/tour-of-beam/backend/integration_tests/function_test.go: ########## @@ -0,0 +1,149 @@ +//go:build integration +// +build integration + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "encoding/json" + "log" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" +) + +const ( + PORT_SDK_LIST = "PORT_SDK_LIST" + PORT_GET_CONTENT_TREE = "PORT_GET_CONTENT_TREE" + PORT_GET_UNIT_CONTENT = "PORT_GET_UNIT_CONTENT" +) + +// scenarios: +// + Get SDK list +// + Get content tree for existing SDK +// - Get content tree for non-existing SDK: 404 Not Found +// - Get unit content for existing SDK, existing unitId +// - Get unit content for non-existing SDK/unitId: 404 Not Found +// TODO: +// - Get content tree for a registered user +// - Get unit content for a registered user +// - Save user code/progress for a registered user +// - (negative) Save user code/progress w/o user token/bad token +// - (negative) Save user code/progress for non-existing SDK/unitId: 404 Not Found + +func loadJson(path string, dst interface{}) error { + fh, err := os.Open(path) + if err != nil { + return err + } + return json.NewDecoder(fh).Decode(dst) +} + +func TestSdkList(t *testing.T) { + port := os.Getenv(PORT_SDK_LIST) + if port == "" { + log.Fatal(PORT_SDK_LIST, "env not set") Review Comment: Same suggestion for rest of the tests as well ########## .github/workflows/tour_of_beam_backend_integration.yml: ########## @@ -0,0 +1,95 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# To learn more about GitHub Actions in Apache Beam check the CI.md + +name: Tour of Beam Go integration tests + +on: + push: + branches: ['master', 'release-*'] + tags: 'v*' + pull_request: + branches: ['master', 'release-*'] + tags: 'v*' + paths: ['learning/tour-of-beam/backend/**'] + +# This allows a subsequently queued workflow run to interrupt previous runs +concurrency: + group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}' + cancel-in-progress: true + +env: + TOB_LEARNING_ROOT: ./samples/learning-content + DATASTORE_PROJECT_ID: test-proj + DATASTORE_EMULATOR_HOST: localhost:8081 + DATASTORE_EMULATOR_DATADIR: ./datadir + PORT_SDK_LIST: 8801 + PORT_GET_CONTENT_TREE: 8802 + PORT_GET_UNIT_CONTENT: 8803 + + +jobs: + integration: + runs-on: ubuntu-latest + defaults: + run: + working-directory: ./learning/tour-of-beam/backend + steps: + - uses: actions/checkout@v3 + - uses: actions/setup-go@v3 + with: + go-version: '1.16' Review Comment: Minor: Could we add a comment explaining we're pinning to 1.16 because Google Cloud Functions don't support higher versions (ideally here and in tour_of_beam_backend.yml)? That way we avoid a well meaning contributor (or dependabot) bumping the version. ########## .github/workflows/tour_of_beam_backend.yml: ########## @@ -28,6 +28,11 @@ on: tags: 'v*' paths: ['learning/tour-of-beam/backend/**'] +# This allows a subsequently queued workflow run to interrupt previous runs +concurrency: + group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}' + cancel-in-progress: true Review Comment: I wasn't familiar with this feature - neat! ########## learning/tour-of-beam/backend/internal/storage/image/Dockerfile: ########## @@ -24,7 +24,7 @@ FROM google/cloud-sdk:$GCLOUD_SDK_VERSION VOLUME /opt/data # RUN mkdir -p /opt/data/WEB-INF -COPY index.yaml /opt/data/WEB-INF/index.yaml +# COPY index.yaml /opt/data/WEB-INF/index.yaml Review Comment: Should we just remove this entirely? Why did we end up moving this? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
