You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/08/12 18:12:50 UTC

[GitHub] [trafficcontrol] ocket8888 opened a new pull request #4948: Unit test checks

ocket8888 opened a new pull request #4948:
URL: https://github.com/apache/trafficcontrol/pull/4948


   ## What does this PR (Pull Request) do?
   - [x] This PR not related to any Issue
   
   This PR will run Go unit tests on all PRs that modify go code, as well as all pushes to any branch that modifies go code. Specifically, it checks modification in paths that match:
   ```gitignore
   grove/**.go
   lib/**.go
   traffic_monitor/**.go
   traffic_ops/traffic_ops_golang/**.go
   traffic_ops_ort/atstccfg/**.go
   ```
   and runs the unit tests in those same paths when modifications are found.
   
   I know that this sort of got shut down on the mailing list, but [that discussion](https://lists.apache.org/thread.html/ec503661975dc9eb0c3719bcaab3b758b5b7489ebec93f95f7f9416b%40%3Cdev.trafficcontrol.apache.org%3E)  was about a year ago now, and we still don't have unit tests running on our PRs. So this PR adds them. If we want to merge it.
   
   ## Which Traffic Control components are affected by this PR?
   none
   
   ## What is the best way to verify this PR?
   Look at the "Checks" tab.
   
   ## The following criteria are ALL met by this PR
   - [x] This PR includes tests
   - [x] Documentation is unnecessary
   - [x] An update to CHANGELOG.md is not necessary
   - [x] This PR includes any and all required license headers
   - [x] This PR does not include a database migration
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4948: Unit test checks

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4948:
URL: https://github.com/apache/trafficcontrol/pull/4948#discussion_r469548727



##########
File path: .github/actions/go-test/Dockerfile
##########
@@ -0,0 +1,23 @@
+# 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.
+
+FROM golang:1.14

Review comment:
       To be consistent with the rest of the repo (which is outdated). Ideally, updating the repo's go version should be changing 1 file in the repo that is referenced from several places (like BUILD_NUMBER), but we're not there yet.
   
   Until then, we should be able to look at the last commit that updated a go version in the repo as a guide for which files to change the next time we update a go version. If we go by minor version in some places and patch version in others, we can't do that.
   
   If you'd like to avoid having 1 more place to change the go version, maybe this Action can run the unit test service in [/tools/golang](https://github.com/apache/trafficcontrol/tree/82452be484/tools/golang).




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] zrhoffman commented on pull request #4948: Unit test checks

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4948:
URL: https://github.com/apache/trafficcontrol/pull/4948#issuecomment-673589610


   retest this please


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4948: Unit test checks

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4948:
URL: https://github.com/apache/trafficcontrol/pull/4948#discussion_r469521604



##########
File path: .github/actions/go-test/entrypoint.sh
##########
@@ -0,0 +1,35 @@
+#!/bin/sh -l

Review comment:
       Dunno, I copied that line from the GHA example code. So at worst I think it shouldn't hurt anything.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4948: Unit test checks

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4948:
URL: https://github.com/apache/trafficcontrol/pull/4948#discussion_r469571753



##########
File path: .github/actions/go-test/Dockerfile
##########
@@ -0,0 +1,23 @@
+# 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.
+
+FROM golang:1.14

Review comment:
       seems easier to just add `.2`




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4948: Unit test checks

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4948:
URL: https://github.com/apache/trafficcontrol/pull/4948#discussion_r469521992



##########
File path: .github/actions/go-test/Dockerfile
##########
@@ -0,0 +1,23 @@
+# 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.
+
+FROM golang:1.14

Review comment:
       well isn't the last number subject to change with security patches? Why are we pinning to a micro version?

##########
File path: .github/workflows/go.unit.tests.yaml
##########
@@ -0,0 +1,48 @@
+# 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.
+
+name: Go Unit Tests
+
+on:
+  push:
+    paths:
+      - grove/**.go
+      - lib/**.go
+      - traffic_monitor/**.go
+      - traffic_ops/traffic_ops_golang/**.go
+      - traffic_ops_ort/atstccfg/**.go
+  create:
+  pull_request:
+    paths:
+      - grove/**.go
+      - lib/**.go
+      - traffic_monitor/**.go
+      - traffic_ops/traffic_ops_golang/**.go
+      - traffic_ops_ort/atstccfg/**.go
+    types: [opened, reopened, edited, synchronize]
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+
+    steps:
+    - name: Checkout
+      uses: actions/checkout@master
+    - name: Run unit tests
+      uses: ./.github/actions/go-test
+      with:
+        dir: ./grove/... ./lib/... ./traffic_monitor/... ./traffic_ops/traffic_ops_golang/... ./traffic_ops_ort/atstccfg/...

Review comment:
       Does TS have tests?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4948: Unit test checks

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4948:
URL: https://github.com/apache/trafficcontrol/pull/4948#discussion_r469471211



##########
File path: .github/actions/go-test/Dockerfile
##########
@@ -0,0 +1,23 @@
+# 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.
+
+FROM golang:1.14

Review comment:
       This should be the exact go version that we require to build ATC (1.14.2 currently)

##########
File path: .github/actions/go-test/entrypoint.sh
##########
@@ -0,0 +1,35 @@
+#!/bin/sh -l

Review comment:
       Does the script need to run in a login shell?

##########
File path: .github/workflows/go.unit.tests.yaml
##########
@@ -0,0 +1,48 @@
+# 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.
+
+name: Go Unit Tests
+
+on:
+  push:
+    paths:
+      - grove/**.go
+      - lib/**.go
+      - traffic_monitor/**.go
+      - traffic_ops/traffic_ops_golang/**.go
+      - traffic_ops_ort/atstccfg/**.go
+  create:
+  pull_request:
+    paths:
+      - grove/**.go
+      - lib/**.go
+      - traffic_monitor/**.go
+      - traffic_ops/traffic_ops_golang/**.go
+      - traffic_ops_ort/atstccfg/**.go

Review comment:
       This should include Traffic Stats sources, too

##########
File path: .github/workflows/go.unit.tests.yaml
##########
@@ -0,0 +1,48 @@
+# 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.
+
+name: Go Unit Tests
+
+on:
+  push:
+    paths:
+      - grove/**.go
+      - lib/**.go
+      - traffic_monitor/**.go
+      - traffic_ops/traffic_ops_golang/**.go
+      - traffic_ops_ort/atstccfg/**.go

Review comment:
       This should include Traffic Stats sources

##########
File path: .github/workflows/go.unit.tests.yaml
##########
@@ -0,0 +1,48 @@
+# 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.
+
+name: Go Unit Tests
+
+on:
+  push:
+    paths:
+      - grove/**.go
+      - lib/**.go
+      - traffic_monitor/**.go
+      - traffic_ops/traffic_ops_golang/**.go
+      - traffic_ops_ort/atstccfg/**.go
+  create:
+  pull_request:
+    paths:
+      - grove/**.go
+      - lib/**.go
+      - traffic_monitor/**.go
+      - traffic_ops/traffic_ops_golang/**.go
+      - traffic_ops_ort/atstccfg/**.go
+    types: [opened, reopened, edited, synchronize]
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+
+    steps:
+    - name: Checkout
+      uses: actions/checkout@master
+    - name: Run unit tests
+      uses: ./.github/actions/go-test
+      with:
+        dir: ./grove/... ./lib/... ./traffic_monitor/... ./traffic_ops/traffic_ops_golang/... ./traffic_ops_ort/atstccfg/...

Review comment:
       Needs to include Traffic Stats

##########
File path: .github/actions/go-test/entrypoint.sh
##########
@@ -0,0 +1,35 @@
+#!/bin/sh -l
+# 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.
+
+set -e
+
+if [ -z "$INPUT_DIR" ]; then
+	# There's a bug in "defaults" for inputs
+	INPUT_DIR="./lib/..."
+fi
+
+GOPATH="$(mktemp -d)"
+SRCDIR="$GOPATH/src/github.com/apache"
+mkdir -p "$SRCDIR"
+ln -s "$PWD" "$SRCDIR/trafficcontrol"
+cd "$SRCDIR/trafficcontrol"
+
+# Need to fetch golang.org/x/* dependencies
+/usr/local/go/bin/go get -v $INPUT_DIR
+/usr/local/go/bin/go test -v $INPUT_DIR
+exit $?

Review comment:
       Is `exit $?` necessary? The exit code isn't printed either way.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] mitchell852 merged pull request #4948: Unit test checks

Posted by GitBox <gi...@apache.org>.
mitchell852 merged pull request #4948:
URL: https://github.com/apache/trafficcontrol/pull/4948


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4948: Unit test checks

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4948:
URL: https://github.com/apache/trafficcontrol/pull/4948#discussion_r469534018



##########
File path: .github/workflows/go.unit.tests.yaml
##########
@@ -0,0 +1,48 @@
+# 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.
+
+name: Go Unit Tests
+
+on:
+  push:
+    paths:
+      - grove/**.go
+      - lib/**.go
+      - traffic_monitor/**.go
+      - traffic_ops/traffic_ops_golang/**.go
+      - traffic_ops_ort/atstccfg/**.go
+  create:
+  pull_request:
+    paths:
+      - grove/**.go
+      - lib/**.go
+      - traffic_monitor/**.go
+      - traffic_ops/traffic_ops_golang/**.go
+      - traffic_ops_ort/atstccfg/**.go
+    types: [opened, reopened, edited, synchronize]
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+
+    steps:
+    - name: Checkout
+      uses: actions/checkout@master
+    - name: Run unit tests
+      uses: ./.github/actions/go-test
+      with:
+        dir: ./grove/... ./lib/... ./traffic_monitor/... ./traffic_ops/traffic_ops_golang/... ./traffic_ops_ort/atstccfg/...

Review comment:
       It's got 1
   
   [/traffic_stats/influxdb_tools/sync/sync_test.go](https://github.com/apache/trafficcontrol/blob/82452be484/traffic_stats/influxdb_tools/sync/sync_test.go)




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4948: Unit test checks

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4948:
URL: https://github.com/apache/trafficcontrol/pull/4948#discussion_r469522900



##########
File path: .github/actions/go-test/entrypoint.sh
##########
@@ -0,0 +1,35 @@
+#!/bin/sh -l
+# 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.
+
+set -e
+
+if [ -z "$INPUT_DIR" ]; then
+	# There's a bug in "defaults" for inputs
+	INPUT_DIR="./lib/..."
+fi
+
+GOPATH="$(mktemp -d)"
+SRCDIR="$GOPATH/src/github.com/apache"
+mkdir -p "$SRCDIR"
+ln -s "$PWD" "$SRCDIR/trafficcontrol"
+cd "$SRCDIR/trafficcontrol"
+
+# Need to fetch golang.org/x/* dependencies
+/usr/local/go/bin/go get -v $INPUT_DIR
+/usr/local/go/bin/go test -v $INPUT_DIR
+exit $?

Review comment:
       the test fails or passes based on the script's exit code. It's not about printing it, it's about exiting with the exact exit code of `go test`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org