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.

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

apps/api testing script fixes #3284

Open
infotroph opened this issue Apr 5, 2024 · 19 comments
Open

apps/api testing script fixes #3284

infotroph opened this issue Apr 5, 2024 · 19 comments

Comments

@infotroph
Copy link
Member

#3283 changed URLs inside test_pecanapi.sh from localhost:8000 to pecan.localhost, but that script is intended for use inside the Docker image (where localhost is the right hostname to use). However it appears the script wasn't fully working before that change, so let's use this issue to plan the next updates before rolling back any of #3283.

Summary originally posted by @Sweetdevil144 in #3283 (comment) :

I think we would be better off with Partially reverting our changes from pecan.localhost to localhost:8000 with following points in consideration :

  • For many of our test cases within apps/api/tests/ folder, pecan.localhost works the same as localhost:8000, only locally however. Can you confirm whether this is true again @infotroph ?
  • Some few selected files return either 400, 401, 404 or do not GET at all and take infinite time causing system barriers to exist.

Below is a description of all test cases which have been causing problems with current Repository changes. (I plan to revert changes from pecan.localhost back to localhost:8000 and then confirm it once again whether these runs are successful or not) :

  • apps/api/tests/test.auth.R : Following test cases
   test_that("Using incorrect username & password returns Status 401", {
  res <- httr::GET(
    "http://pecan.localhost/api/models/",
    httr::authenticate("wrong_username_user", "wrong_password_user")
  )
  # return 200 when it should return 401. (Auth errors)
  expect_equal(res$status, 401)
})

test_that("Not using username & password returns Status 401", {
  res <- httr::GET(
    "http://pecan.localhost/api/models/",
  )
  # return 200 when it should return 401. (Auth errors)
  expect_equal(res$status, 401)
})
  • apps/api/tests/test.inputs.R : Following test cases
test_that("Calling /api/inputs/{input_id} with valid parameters returns Status 200", {
  res <- httr::GET(
    paste0("http://pecan.localhost/api/inputs/", 99000000003),
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 200)
})

test_that("Calling /api/inputs/{input_id} with invalid parameters returns Status 404", {
  res <- httr::GET(
    "http://pecan.localhost/api/inputs/0",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 404)
})

test_that("Calling /api/inputs/{input_id}?filename={filename} with invalid parameters returns Status 404", {
  res <- httr::GET(
    "http://pecan.localhost/api/inputs/295?filename=random",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 400)
})
  • apps/api/tests/test.runs.R : Following test cases
test_that("Calling /api/runs/{run_id}/graph/{year}/{yvar}/ with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://pecan.localhost/api/runs/99000000282/graph/2002/GPP",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 200)
})

test_that("Calling /api/runs/{run_id}/graph/{year}/{yvar}/ with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://pecan.localhost/api/runs/1000000000/graph/100/GPP",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 404)
})

test_that("Calling /api/runs/{run_id}/input/{filename} with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://pecan.localhost/api/runs/99000000282/input/sipnet.in",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 200)
})

test_that("Calling /api/runs/{run_id}/input/{filename} with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://pecan.localhost/api/runs/1000000000/input/randomfile",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 404)
})

test_that("Calling /api/runs/{run_id}/output/{filename} with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://pecan.localhost/api/runs/99000000282/output/2002.nc",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 200)
})

test_that("Calling /api/runs/{run_id}/output/{filename} with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://pecan.localhost/api/runs/1000000000/output/randomfile",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 404)
})
  • apps/api/tests/test.status.R : Following test cases
# Old file : 
test_that("Calling /api/status returns Status 200", {
# SPace was present in the GET command leading to failure of this test
  res <- httr::GET(" http://pecan.localhost/api/status")
  expect_equal(res$status, 200)
})
  • apps/api/tests/test.workflows.R : Following test cases
test_that("Submitting XML workflow to /api/workflows/ returns Status 201", {
  # Utilizing absolute path for localhost docker testing instead of 'test_workflows/api.sipnet.xml'
  xml_string <- paste0(xml2::read_xml("apps/api/tests/test_workflows/api.sipnet.xml"))
  res <- httr::POST(
    "http://pecan.localhost/api/workflows/",
    httr::authenticate("carya", "illinois"),
    httr::content_type("application/xml"),
    body = xml_string
  )
  # No Response
  expect_equal(res$status, 201)
})

test_that("Submitting JSON workflow to /api/workflows/ returns Status 201", {
  Sys.sleep(2)
  json_workflow <- jsonlite::read_json("test_workflows/api.sipnet.json")
  res <- httr::POST(
    "http://pecan.localhost/api/workflows/",
    httr::authenticate("carya", "illinois"),
    body = json_workflow,
    encode='json'
  )
  # No Response
  expect_equal(res$status, 201)
})

test_that("Calling /api/workflows/{id}/file/{filename} with valid parameters returns Status 200", {
  res <- httr::GET(
    paste0("http://pecan.localhost/api/workflows/", 99000000031, "/file/", "pecan.CONFIGS.xml"),
    httr::authenticate("carya", "illinois")
  )
  # No Response
  expect_equal(res$status, 200)
})

test_that("Calling /api/workflows/{id}/file/{filename} with invalid parameters returns Status 404", {
  res <- httr::GET(
    "http://pecan.localhost/api/workflows/0/file/randomfile.txt",
    httr::authenticate("carya", "illinois")
  )
  # No Response
  expect_equal(res$status, 404)
})
@infotroph infotroph mentioned this issue Apr 5, 2024
13 tasks
@infotroph
Copy link
Member Author

In case it helps, here's how I'm currently running tests. To be clear, this is just the first way I tried that worked and there may well be better approaches.

The key reason for the complexity in my approach is that the default Docker Compose configuration doesn't run the API image from a shared volume, meaning any changes you make in your local apps/api won't show up in pecan-api-1 until you rebuild and restart the image. It would be possible to change this, but I wanted to get the existing tests working with as few changes as possible.

  • Make sure the local Docker Compose stack is running at least the postgres service. We'll manually add our test version of the api image to the same network.
  • Make sure the users table contains a user named carya with password illinois. If the rest of PEcAn is working normally for you then it's probably there, but mine was missing today (my fault -- previous shenanigans with my test db) and I was puzzled for a while why the login wasn't working.
  • Edit line 6 of test_pecanapi.sh to change http://pecan.localhost/ to http://localhost:8000, otherwise the test script waits forever thinking the server hasn't started yet. I think this was a mis-fix that all of us (including me as the PR approver!) missed in fixes issues #2955 and #2959 #3010.
  • Build image: cd apps/api && docker build -t apitest --build-arg IMAGE_VERSION=develop . The IMAGE_VERSION arg is to use the up-to-date pecan/base:develop as the base image rather than pecan/base:latest, which points to the now-very-outdated 1.7.2 release.
  • Run tests: docker run -it --rm --network pecan_pecan -w /api apitest ./test_pecanapi.sh.

@infotroph
Copy link
Member Author

infotroph commented Apr 5, 2024

When I run the above against commit 1bf1582 (head of develop just before #3283 was merged), it runs and passes the tests for auth and formats, then hangs on the 3rd through last tests in inputs, and if I comment those out it passes all tests for models, PFTs, ping, and the first three tests of runs before throwing a series of errors about missing IDs.

Comparing URL patterns in the passing and failing tests of test.inputs.R and test.runs.R, it looks like maybe URLs that pass IDs as query parameters (e.g. http://localhost:8000/api/foo/?id=100) are working correctly, but those that include them as paths (e.g. http://localhost:8000/api/foo/100) are not. I haven't debugged this any further yet, but I don't think those failures are the fault of how we run the test script.

@Sweetdevil144
Copy link
Contributor

Sweetdevil144 commented Apr 8, 2024

it looks like maybe URLs that pass IDs as query parameters (e.g. http://localhost:8000/api/foo/?id=100) are working correctly, but those that include them as paths (e.g. http://localhost:8000/api/foo/100) are not.

This seems to be correct in our case. When we don't employ usage of ID parameters, it does not even returns a response. However, we can still get a status response with usage of ID Parameters. I guess the main issue lies due to aging of our test cases and no being able to line up with our Latest API changes

@Sweetdevil144
Copy link
Contributor

Sweetdevil144 commented Apr 9, 2024

Correct me if I'm wrong. In the auth section, I don't think we require authorisation to GET our models via :

res <- httr::GET(
    "http://pecan.localhost/api/models/",
    # Test Failed
)

However, Authentication is required on using :

res <- httr::GET(
    "http://localhost:8000/api/models/",
    # Test Passed
)

@Sweetdevil144
Copy link
Contributor

Sweetdevil144 commented Apr 9, 2024

I still think that the below test is useless since no timeout/endpoint has been set for such responses within our test.inputs.R

test_that("Calling /api/inputs/{input_id} with invalid parameters returns Status 404", {
  res <- httr::GET(
    "http://localhost:8000/api/inputs/0",
    httr::authenticate("carya", "illinois")
  )
  # No Response. Stops the testing Permanently
  expect_equal(res$status, 404)
})

Another potential issue may be lack of eror handeling and res$statusCode updation within our search.inputs.R in rpecanapi as below :

if(!is.null(model_id)) {
    url <- paste0(url, "&model_id=", model_id)
  }
  if(!is.null(site_id)) {
    url <- paste0(url, "&site_id=", site_id)
  }
  if(!is.null(format_id)) {
    url <- paste0(url, "&format_id=", format_id)
  }
  if(!is.null(host_id)) {
    url <- paste0(url, "&host_id=", host_id)
  }

I'm still not a 100% sure about this idea so please correct me if I'm not going thoroughly.

@Sweetdevil144
Copy link
Contributor

Sweetdevil144 commented Apr 9, 2024

Status Update for test.input.R :

test_that("Calling /api/inputs/{input_id}?filename={filename} with valid parameters returns Status 200", {
  res <- httr::GET(
    paste0("http://localhost:8000/api/inputs/295?filename=fraction.plantation"),
    httr::authenticate("carya", "illinois")
  )
  # No response. Stops the testing Permanently
  expect_equal(res$status, 200)
})

test_that("Calling /api/inputs/{input_id}?filename={filename} with invalid parameters returns Status 404", {
  res <- httr::GET(
    "http://localhost:8000/api/inputs/295?filename=random",
    httr::authenticate("carya", "illinois")
  )
  # No Response. Stops the testing Permanently
  expect_equal(res$status, 400)
})
test_that("Calling /api/inputs/{input_id} with valid parameters returns Status 200", {
  res <- httr::GET(
    paste0("http://localhost:8000/api/inputs/", 99000000003),
    httr::authenticate("carya", "illinois")
  )
  # No Response. Stops the testing Permanently
  expect_equal(res$status, 200)
})

Are the related endpoints correctly created and linked? If not, it would be natural to return No Response. If yes, there may be any underlying bug in our rpecanapi causing this.

@Sweetdevil144
Copy link
Contributor

Sweetdevil144 commented Apr 9, 2024

Just a preview from our Swagger Documentations about responses. Making sure the current endpoints and documentations are up to date.


Screenshot 2024-04-10 at 3 05 55 AM Screenshot 2024-04-10 at 3 04 26 AM

EDIT : Another Point to be Noted : No endpoint such as /api/inputs/{input_id}?filename={filename} exists within our documentation. Leaving No possible means to confirm the scenarios of testing.

Screenshot 2024-04-10 at 3 12 06 AM

Maybe these lines from our download.input.R from rpecanapi may help :

expr = {
      url <- paste0(server$url, "/api/inputs/", input_id)
      if(! is.null(filename)) {
        url <- paste0(url, "?filename=", filename)
      }
      
      if(! is.null(server$username) && ! is.null(server$password)){
        res <- httr::GET(
          url,
          httr::authenticate(server$username, server$password)
        )
      }
      else{
        res <- httr::GET(url)
      }
    }
    # Rest of Code

@Sweetdevil144
Copy link
Contributor

This is the test case I've been looking into for now in apps/api/tests/test.runs.R:

test_that("Calling /api/runs/{run_id}/graph/{year}/{yvar}/ with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://localhost:8000/api/runs/99000000282/graph/2002/GPP",
    httr::authenticate("carya", "illinois")
  )
  # No Response
  expect_equal(res$status, 200)
})

I was looking within our SwaggerUI link and found its utilisation URL as follow :
/api/runs/{run_id}/graph/{year}/{y_var} . Seems good, yet returns no response because the redirection URL in example test was as follow within our SwaggerUI :
https://pecan-dev.ncsa.illinois.edu/api/runs/99000000282/graph/2002/GPP?x_var=time&width=800&height=600.

Two points I wanted clarity on :

  • Which of the above link is correct?
  • If the first link is the correct one, then why were we not utilising parameters like x_var, width and height in our test case earlier?

@Sweetdevil144
Copy link
Contributor

Any Updates/Reviews @infotroph and @mdietze ??

@Sweetdevil144
Copy link
Contributor

Final briefing of all failing test cases with error logs/responses can be viewed in this test-branch's folder: https://github.com/Sweetdevil144/pecan/tree/test-branch/apps/api/tests

Started with debugging the corresponding rpecanapi folders and code files.

@Sweetdevil144
Copy link
Contributor

Sweetdevil144 commented Apr 24, 2024

Final summary of total passing results {excluding failing(commented out) tests suites} :

| F W S  OK | Context
✔ |         3 | Testing authentication for API [96.2s]                                                                            
✔ |         4 | Testing all formats related endpoints [109.1s]                                                                    
✔ |         2 | Testing all inputs related endpoints [127.1s]                                                                     
✔ |         4 | Testing all models endpoints [13.6s]                                                                              
✔ |         4 | Testing all PFTs endpoints [13.7s]                                                                                
✔ |         1 | Testing the /api/ping endpoint                                                                                    
✔ |         3 | Testing all runs endpoints [7.3s]                                                                                 
✔ |         4 | Testing all sites endpoints [4.8s]                                                                                
✔ |         1 | Testing the /api/status endpoint [0.4s]                                                                           
✔ |         5 | Testing all workflows endpoints [10.3s] 
✔ |         5 | Testing all workflows endpoints [10.3s]                                                                           
⠏ |         0 | testthat                                                                                                          
══ Results ═══════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 389.7 s

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 31 ]

@infotroph
Copy link
Member Author

Sorry for the long wait. It’s interesting that the suite takes so long even with failing tests commented out — are the long run times in the first three tests related to the auth issues you mentioned?

@Sweetdevil144
Copy link
Contributor

Sorry for the long wait. It’s interesting that the suite takes so long even with failing tests commented out — are the long run times in the first three tests related to the auth issues you mentioned?

I don't think so. It's just that my host machine has been bearing too much of a load recently and is underperformed. When it's only running docker, than it quickly finishes the test suites in about a minute or two.

@allgandalf
Copy link
Collaborator

I don't think so. It's just that my host machine has been bearing too much of a load recently and is underperformed.

If you decide to fix this, I can assign you a high end VM at my companies end for you to work faster, would that work for you ? @Sweetdevil144

@Sweetdevil144
Copy link
Contributor

If you decide to fix this, I can assign you a high end VM at my companies end for you to work faster, would that work for you ?

Yeah sure. No Problem with that. However, I think it isn't much big of a problem. I'd been running some high usage processes alongside pecan in docker at the time of testing, which caused the test to take lot of time. But I'd love to try the VM too.

@infotroph
Copy link
Member Author

@GandalfGwaihir That's a generous offer! I trust that you

  • Have checked that this is within the bounds of your company's resource usage policies, and
  • Understand that this will be a deal between your company and the two of you as individuals, not the PEcAn organization?

@allgandalf
Copy link
Collaborator

@infotroph , yeah no worries, also I confirm that this is an individual offer not related to PEcAn organisation!

@Sweetdevil144 ping me on slack with your system config, we're take it on personal DM from here

@allgandalf
Copy link
Collaborator

Assigned a VM , @Sweetdevil144 can you put in a timeline to solve this issue please ?

@Sweetdevil144
Copy link
Contributor

Hey, thanks a lot @GandalfGwaihir . It'll surely help a lot. As I estimate right now, this issue will be resolved by the end of this month. Although, I'll try to wrap it up as soon as possible.
Also, Regarding this issue, @infotroph do you have any opinion regarding this thread? As far as it goes for the rpecanapi, I'm still debugging the roots of our issues. (Although I highly doubt what I presented above MAY be the reason for such high failure rates)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: To do
Development

No branches or pull requests

4 participants