You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/06/01 17:45:41 UTC
[GitHub] [beam] lnogueir opened a new pull request, #17803: [BEAM-14532] Add integration testing to fhirio Read transform
lnogueir opened a new pull request, #17803:
URL: https://github.com/apache/beam/pull/17803
Follow up to (apache#17748)[https://github.com/apache/beam/pull/17748] and it consists of adding integration testing to the Read transform in fhirio package.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] youngoli commented on pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
youngoli commented on PR #17803:
URL: https://github.com/apache/beam/pull/17803#issuecomment-1150668874
Run Go Postcommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] youngoli commented on a diff in pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
youngoli commented on code in PR #17803:
URL: https://github.com/apache/beam/pull/17803#discussion_r893068142
##########
sdks/go/test/integration/io/fhirio/fhirio_test.go:
##########
@@ -0,0 +1,198 @@
+// 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.
+
+package fhirio
+
+import (
+ "bytes"
+ "context"
+ "crypto/rand"
+ "encoding/json"
+ "errors"
+ "flag"
+ "fmt"
+ "math/big"
+ "os"
+ "strconv"
+ "strings"
+ "testing"
+ "time"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/io/fhirio"
+ _ "github.com/apache/beam/sdks/v2/go/pkg/beam/runners/dataflow"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/testing/ptest"
+ "github.com/apache/beam/sdks/v2/go/test/integration"
+ "google.golang.org/api/healthcare/v1"
+ "google.golang.org/api/option"
+)
+
+const (
+ datasetPathFmt = "projects/%s/locations/%s/datasets/apache-beam-integration-testing"
+ testDataDir = "../../../../data/fhir_bundles/"
+)
+
+var (
+ gcpProject string
+ gcpRegion string
+ storeService *healthcare.ProjectsLocationsDatasetsFhirStoresFhirService
+ storeManagementService *healthcare.ProjectsLocationsDatasetsFhirStoresService
+)
+
+func checkFlags(t *testing.T) {
+ if gcpProject == "" || gcpRegion == "" {
+ t.Skip("GCP flags not provided.")
+ }
+}
+
+// Sets up a test fhir store by creating and populating data to it for testing
+// purposes. It returns the name of the created store path, a slice of the
+// resource paths to be used in tests, and a function to teardown what has been
+// set up.
+func setupFhirStore(t *testing.T) (string, []string, func()) {
+ t.Helper()
+ if storeService == nil || storeManagementService == nil {
+ t.Fatal("Healthcare Services were not initialized")
+ }
+
+ healthcareDataset := fmt.Sprintf(datasetPathFmt, gcpProject, gcpRegion)
Review Comment:
Ok I just checked, the direct runner integration tests aren't handled by any of our postcommits which might be an oversight. The precommit does technically test this directory, but it's just doing a plain "go test ./..." command, so it doesn't provide any of the necessary flags (it's technically just aimed at unit 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lnogueir commented on pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
lnogueir commented on PR #17803:
URL: https://github.com/apache/beam/pull/17803#issuecomment-1143958915
The 2 test files I added in `sdks/go/data/healthcare/R4` already exist in this repository under `sdks/java/io/google-cloud-platform/src/test/resources/R4`. Is there any location for test files to be shared between the SDKs? That way we don't have to duplicate them.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] commented on pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #17803:
URL: https://github.com/apache/beam/pull/17803#issuecomment-1144140687
# [Codecov](https://codecov.io/gh/apache/beam/pull/17803?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#17803](https://codecov.io/gh/apache/beam/pull/17803?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fecb15b) into [master](https://codecov.io/gh/apache/beam/commit/4e2565a2848f84a28746ace0287800af3e93a2e4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4e2565a) will **increase** coverage by `0.07%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #17803 +/- ##
==========================================
+ Coverage 74.00% 74.07% +0.07%
==========================================
Files 697 697
Lines 91945 91929 -16
==========================================
+ Hits 68045 68100 +55
+ Misses 22653 22584 -69
+ Partials 1247 1245 -2
```
| Flag | Coverage Δ | |
|---|---|---|
| go | `50.74% <0.00%> (+0.23%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/17803?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [sdks/go/pkg/beam/io/fhirio/common.go](https://codecov.io/gh/apache/beam/pull/17803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9pby9maGlyaW8vY29tbW9uLmdv) | `0.00% <0.00%> (ø)` | |
| [sdks/go/pkg/beam/core/runtime/exec/sdf.go](https://codecov.io/gh/apache/beam/pull/17803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy9zZGYuZ28=) | `70.61% <0.00%> (-1.19%)` | :arrow_down: |
| [...o/pkg/beam/io/rtrackers/offsetrange/offsetrange.go](https://codecov.io/gh/apache/beam/pull/17803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9pby9ydHJhY2tlcnMvb2Zmc2V0cmFuZ2Uvb2Zmc2V0cmFuZ2UuZ28=) | `78.50% <0.00%> (+2.80%)` | :arrow_up: |
| [sdks/go/pkg/beam/core/graph/fn.go](https://codecov.io/gh/apache/beam/pull/17803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL2dyYXBoL2ZuLmdv) | `85.33% <0.00%> (+8.29%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/17803?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/17803?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4e2565a...fecb15b](https://codecov.io/gh/apache/beam/pull/17803?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17803:
URL: https://github.com/apache/beam/pull/17803#issuecomment-1143953221
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] youngoli commented on pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
youngoli commented on PR #17803:
URL: https://github.com/apache/beam/pull/17803#issuecomment-1150665026
Run XVR_GoUsingJava_Dataflow PostCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lnogueir commented on pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
lnogueir commented on PR #17803:
URL: https://github.com/apache/beam/pull/17803#issuecomment-1143994814
R: @msbukal
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lnogueir commented on a diff in pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
lnogueir commented on code in PR #17803:
URL: https://github.com/apache/beam/pull/17803#discussion_r893940979
##########
sdks/go/test/integration/io/fhirio/fhirio_test.go:
##########
@@ -0,0 +1,198 @@
+// 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.
+
+package fhirio
+
+import (
+ "bytes"
+ "context"
+ "crypto/rand"
+ "encoding/json"
+ "errors"
+ "flag"
+ "fmt"
+ "math/big"
+ "os"
+ "strconv"
+ "strings"
+ "testing"
+ "time"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/io/fhirio"
+ _ "github.com/apache/beam/sdks/v2/go/pkg/beam/runners/dataflow"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/testing/ptest"
+ "github.com/apache/beam/sdks/v2/go/test/integration"
+ "google.golang.org/api/healthcare/v1"
+ "google.golang.org/api/option"
+)
+
+const (
+ datasetPathFmt = "projects/%s/locations/%s/datasets/apache-beam-integration-testing"
+ testDataDir = "../../../../data/fhir_bundles/"
+)
+
+var (
+ gcpProject string
+ gcpRegion string
+ storeService *healthcare.ProjectsLocationsDatasetsFhirStoresFhirService
+ storeManagementService *healthcare.ProjectsLocationsDatasetsFhirStoresService
+)
+
+func checkFlags(t *testing.T) {
+ if gcpProject == "" || gcpRegion == "" {
+ t.Skip("GCP flags not provided.")
+ }
+}
+
+// Sets up a test fhir store by creating and populating data to it for testing
+// purposes. It returns the name of the created store path, a slice of the
+// resource paths to be used in tests, and a function to teardown what has been
+// set up.
+func setupFhirStore(t *testing.T) (string, []string, func()) {
+ t.Helper()
+ if storeService == nil || storeManagementService == nil {
+ t.Fatal("Healthcare Services were not initialized")
+ }
+
+ healthcareDataset := fmt.Sprintf(datasetPathFmt, gcpProject, gcpRegion)
Review Comment:
Resolving this since as per chat discussion.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17803:
URL: https://github.com/apache/beam/pull/17803#issuecomment-1143953227
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lnogueir commented on pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
lnogueir commented on PR #17803:
URL: https://github.com/apache/beam/pull/17803#issuecomment-1145576396
R: @youngoli
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] youngoli commented on a diff in pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
youngoli commented on code in PR #17803:
URL: https://github.com/apache/beam/pull/17803#discussion_r893053520
##########
sdks/go/test/integration/io/fhirio/fhirio_test.go:
##########
@@ -0,0 +1,198 @@
+// 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.
+
+package fhirio
+
+import (
+ "bytes"
+ "context"
+ "crypto/rand"
+ "encoding/json"
+ "errors"
+ "flag"
+ "fmt"
+ "math/big"
+ "os"
+ "strconv"
+ "strings"
+ "testing"
+ "time"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/io/fhirio"
+ _ "github.com/apache/beam/sdks/v2/go/pkg/beam/runners/dataflow"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/testing/ptest"
+ "github.com/apache/beam/sdks/v2/go/test/integration"
+ "google.golang.org/api/healthcare/v1"
+ "google.golang.org/api/option"
+)
+
+const (
+ datasetPathFmt = "projects/%s/locations/%s/datasets/apache-beam-integration-testing"
+ testDataDir = "../../../../data/fhir_bundles/"
+)
+
+var (
+ gcpProject string
+ gcpRegion string
+ storeService *healthcare.ProjectsLocationsDatasetsFhirStoresFhirService
+ storeManagementService *healthcare.ProjectsLocationsDatasetsFhirStoresService
+)
+
+func checkFlags(t *testing.T) {
+ if gcpProject == "" || gcpRegion == "" {
+ t.Skip("GCP flags not provided.")
+ }
+}
+
+// Sets up a test fhir store by creating and populating data to it for testing
+// purposes. It returns the name of the created store path, a slice of the
+// resource paths to be used in tests, and a function to teardown what has been
+// set up.
+func setupFhirStore(t *testing.T) (string, []string, func()) {
+ t.Helper()
+ if storeService == nil || storeManagementService == nil {
+ t.Fatal("Healthcare Services were not initialized")
+ }
+
+ healthcareDataset := fmt.Sprintf(datasetPathFmt, gcpProject, gcpRegion)
+ createdFhirStore, err := createStore(healthcareDataset)
+ if err != nil {
+ t.Fatal("Test store failed to be created")
+ }
+ createdFhirStorePath := createdFhirStore.Name
+
+ resourcePaths := populateStore(createdFhirStorePath)
+ if len(resourcePaths) == 0 {
+ t.Fatal("No data got populated to test")
+ }
+
+ return createdFhirStorePath, resourcePaths, func() {
+ _, _ = deleteStore(createdFhirStorePath)
+ }
+}
+
+func createStore(dataset string) (*healthcare.FhirStore, error) {
+ randInt, _ := rand.Int(rand.Reader, big.NewInt(32))
+ testFhirStoreId := "FHIR_store_write_it_" + strconv.FormatInt(time.Now().UnixMilli(), 10) + "_" + randInt.String()
+ fhirStore := &healthcare.FhirStore{
+ DisableReferentialIntegrity: true,
+ EnableUpdateCreate: true,
+ Version: "R4",
+ }
+ return storeManagementService.Create(dataset, fhirStore).FhirStoreId(testFhirStoreId).Do()
+}
+
+func deleteStore(storePath string) (*healthcare.Empty, error) {
+ return storeManagementService.Delete(storePath).Do()
+}
+
+// Populates fhir store with data. Note that failure to populate some data is not
+// detrimental to the tests, so it is fine to ignore.
+func populateStore(storePath string) []string {
+ resourcePaths := make([]string, 0)
+ for _, bundle := range readPrettyBundles() {
+ response, err := storeService.ExecuteBundle(storePath, bytes.NewReader(bundle)).Do()
+ if err != nil {
+ continue
+ }
+
+ var body struct {
+ Entry []struct {
+ Response struct {
+ Location string `json:"location"`
+ Status string `json:"status"`
+ } `json:"response"`
+ } `json:"entry"`
+ }
+ err = json.NewDecoder(response.Body).Decode(&body)
+ if err != nil {
+ continue
+ }
+
+ for _, entry := range body.Entry {
+ bundleFailedToBeCreated := !strings.Contains(entry.Response.Status, "201")
+ if bundleFailedToBeCreated {
+ continue
+ }
+
+ resourcePath, err := extractResourcePathFrom(entry.Response.Location)
+ if err != nil {
+ continue
+ }
+ resourcePaths = append(resourcePaths, resourcePath)
+ }
+ }
+ return resourcePaths
+}
+
+func readPrettyBundles() [][]byte {
+ files, _ := os.ReadDir(testDataDir)
+ bundles := make([][]byte, len(files))
+ for i, file := range files {
+ bundles[i], _ = os.ReadFile(testDataDir + file.Name())
+ }
+ return bundles
+}
+
+func extractResourcePathFrom(resourceLocationUrl string) (string, error) {
+ // The resource location url is in the following format:
+ // https://healthcare.googleapis.com/v1/projects/PROJECT_ID/locations/LOCATION/datasets/DATASET_ID/fhirStores/STORE_ID/fhir/RESOURCE_NAME/RESOURCE_ID/_history/HISTORY_ID
+ // But the API calls use this format: projects/PROJECT_ID/locations/LOCATION/datasets/DATASET_ID/fhirStores/STORE_ID/fhir/RESOURCE_NAME/RESOURCE_ID
+ startIdx := strings.Index(resourceLocationUrl, "projects/")
+ endIdx := strings.Index(resourceLocationUrl, "/_history")
+ if startIdx == -1 || endIdx == -1 {
+ return "", errors.New("resource location url is invalid")
+ }
+ return resourceLocationUrl[startIdx:endIdx], nil
+}
+
+func TestFhirIO_Read(t *testing.T) {
+ integration.CheckFilters(t)
+ checkFlags(t)
+
+ _, testResourcePaths, teardownFhirStore := setupFhirStore(t)
+ defer teardownFhirStore()
+
+ p := ReadPipeline(testResourcePaths)
+ ptest.RunAndValidate(t, p)
+}
+
+func TestFhirIO_InvalidRead(t *testing.T) {
+ integration.CheckFilters(t)
+ checkFlags(t)
+
+ fhirStorePath, _, teardownFhirStore := setupFhirStore(t)
+ defer teardownFhirStore()
+
+ p := InvalidReadPipeline(fhirStorePath)
+ ptest.RunAndValidate(t, p)
+}
+
+func TestMain(m *testing.M) {
+ flag.Parse()
+ beam.Init()
+
+ healthcareService, err := healthcare.NewService(context.Background(), option.WithUserAgent(fhirio.UserAgent))
+ if err == nil {
+ storeService = healthcare.NewProjectsLocationsDatasetsFhirStoresFhirService(healthcareService)
+ storeManagementService = healthcare.NewProjectsLocationsDatasetsFhirStoresService(healthcareService)
+ }
+
+ beam.PipelineOptions.LoadOptionsFromFlags(nil)
+ gcpProject = beam.PipelineOptions.Get("project")
Review Comment:
I assume this bit is for checkFlags to work properly? I'm worried there might be unintended issues with calling these methods manually here. Instead could you just check the Project and Region variables from the [gcpopts](https://github.com/apache/beam/blob/master/sdks/go/pkg/beam/options/gcpopts/options.go) package directly wherever you need it? That seems like it would work better.
##########
sdks/go/test/integration/io/fhirio/fhirio_test.go:
##########
@@ -0,0 +1,198 @@
+// 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.
+
+package fhirio
+
+import (
+ "bytes"
+ "context"
+ "crypto/rand"
+ "encoding/json"
+ "errors"
+ "flag"
+ "fmt"
+ "math/big"
+ "os"
+ "strconv"
+ "strings"
+ "testing"
+ "time"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/io/fhirio"
+ _ "github.com/apache/beam/sdks/v2/go/pkg/beam/runners/dataflow"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/testing/ptest"
+ "github.com/apache/beam/sdks/v2/go/test/integration"
+ "google.golang.org/api/healthcare/v1"
+ "google.golang.org/api/option"
+)
+
+const (
+ datasetPathFmt = "projects/%s/locations/%s/datasets/apache-beam-integration-testing"
+ testDataDir = "../../../../data/fhir_bundles/"
+)
+
+var (
+ gcpProject string
+ gcpRegion string
+ storeService *healthcare.ProjectsLocationsDatasetsFhirStoresFhirService
+ storeManagementService *healthcare.ProjectsLocationsDatasetsFhirStoresService
+)
+
+func checkFlags(t *testing.T) {
+ if gcpProject == "" || gcpRegion == "" {
+ t.Skip("GCP flags not provided.")
+ }
+}
+
+// Sets up a test fhir store by creating and populating data to it for testing
+// purposes. It returns the name of the created store path, a slice of the
+// resource paths to be used in tests, and a function to teardown what has been
+// set up.
+func setupFhirStore(t *testing.T) (string, []string, func()) {
+ t.Helper()
+ if storeService == nil || storeManagementService == nil {
+ t.Fatal("Healthcare Services were not initialized")
+ }
+
+ healthcareDataset := fmt.Sprintf(datasetPathFmt, gcpProject, gcpRegion)
Review Comment:
These tests are enabled in direct runner, but this requires a gcp project and gcp region to be specified, which they normally aren't with the direct runner. My first instinct is that these tests wouldn't work when run on the direct runner with our current integration test framework. Have you confirmed whether it does or not?
##########
sdks/go/test/integration/io/fhirio/fhirio.go:
##########
@@ -0,0 +1,30 @@
+package fhirio
+
+import (
+ "strings"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/io/fhirio"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/testing/passert"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/testing/ptest"
+)
+
+func ReadPipeline(testResourcePaths []string) *beam.Pipeline {
Review Comment:
Some of our integration tests even from before my time started this pattern. When I've personally written an integration test I've only followed this pattern when I've needed to execute multiple pipelines in one integration test (for example reading and writing the same batch of elements in two separate pipelines to ensure that they happen sequentially). So basically just following old patterns for consistency reasons.
But revisiting it I agree; put this code in the test method. Or if there's a good reason to have a separate function (like deduplicating code), then put the function in the _test.go file.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lnogueir commented on pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
lnogueir commented on PR #17803:
URL: https://github.com/apache/beam/pull/17803#issuecomment-1143994920
R: @youngoli
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #17803:
URL: https://github.com/apache/beam/pull/17803#issuecomment-1143998029
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] msbukal commented on a diff in pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
msbukal commented on code in PR #17803:
URL: https://github.com/apache/beam/pull/17803#discussion_r888142695
##########
sdks/go/test/integration/io/fhirio/fhirio.go:
##########
@@ -0,0 +1,30 @@
+package fhirio
+
+import (
+ "strings"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/io/fhirio"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/testing/passert"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/testing/ptest"
+)
+
+func ReadPipeline(testResourcePaths []string) *beam.Pipeline {
Review Comment:
Go style usually actively disapproves of running test logic in a separate, non-test method. Is there a reason you do a pattern like this?
Otherwise it should just go in TestRead_*
##########
sdks/go/pkg/beam/io/fhirio/read_test.go:
##########
@@ -76,15 +76,20 @@ func TestRead(t *testing.T) {
})
pipelineResult := ptest.RunAndValidate(t, p)
counterResults := pipelineResult.Metrics().AllMetrics().Counters()
- if len(counterResults) != 1 {
- t.Fatal("Only one counter should have been used")
- }
- if counterResults[0].Name() != "fhirio/read_resource_error_count" {
- t.Fatal("Only error counter should have been used")
- }
- if counterResults[0].Result() != int64(len(testResourcePaths)) {
- t.Fatal("Counter should have been incremented by the number of test resource paths")
+ if counterResult := counterResults[0]; len(counterResults) == 1 {
Review Comment:
go style is more like the left, you can just move up the assignment like
```
counterResult := counterResults[0]
if len(counterResults) != 1 {
t.Fatalf(...)
}
[other validation]
```
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] youngoli commented on a diff in pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
youngoli commented on code in PR #17803:
URL: https://github.com/apache/beam/pull/17803#discussion_r893070054
##########
sdks/go/test/integration/io/fhirio/fhirio_test.go:
##########
@@ -0,0 +1,198 @@
+// 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.
+
+package fhirio
+
+import (
+ "bytes"
+ "context"
+ "crypto/rand"
+ "encoding/json"
+ "errors"
+ "flag"
+ "fmt"
+ "math/big"
+ "os"
+ "strconv"
+ "strings"
+ "testing"
+ "time"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/io/fhirio"
+ _ "github.com/apache/beam/sdks/v2/go/pkg/beam/runners/dataflow"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/testing/ptest"
+ "github.com/apache/beam/sdks/v2/go/test/integration"
+ "google.golang.org/api/healthcare/v1"
+ "google.golang.org/api/option"
+)
+
+const (
+ datasetPathFmt = "projects/%s/locations/%s/datasets/apache-beam-integration-testing"
+ testDataDir = "../../../../data/fhir_bundles/"
+)
+
+var (
+ gcpProject string
+ gcpRegion string
+ storeService *healthcare.ProjectsLocationsDatasetsFhirStoresFhirService
+ storeManagementService *healthcare.ProjectsLocationsDatasetsFhirStoresService
+)
+
+func checkFlags(t *testing.T) {
+ if gcpProject == "" || gcpRegion == "" {
+ t.Skip("GCP flags not provided.")
+ }
+}
+
+// Sets up a test fhir store by creating and populating data to it for testing
+// purposes. It returns the name of the created store path, a slice of the
+// resource paths to be used in tests, and a function to teardown what has been
+// set up.
+func setupFhirStore(t *testing.T) (string, []string, func()) {
+ t.Helper()
+ if storeService == nil || storeManagementService == nil {
+ t.Fatal("Healthcare Services were not initialized")
+ }
+
+ healthcareDataset := fmt.Sprintf(datasetPathFmt, gcpProject, gcpRegion)
Review Comment:
Last comment, but I think what you should aim at supporting is the portable runner, which is basically the more fully featured "direct" runner. And you might need to pipe a flag directly to the test by [adding something here](https://github.com/apache/beam/blob/master/sdks/go/test/build.gradle#L147).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] youngoli merged pull request #17803: [BEAM-14532] Add integration testing to fhirio Read transform
Posted by GitBox <gi...@apache.org>.
youngoli merged PR #17803:
URL: https://github.com/apache/beam/pull/17803
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org