- 
                Notifications
    You must be signed in to change notification settings 
- Fork 50
test(e2e): add e2e test for interaction with ArgoCD API #549
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
Conversation
| Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@            Coverage Diff             @@
##             main     #549      +/-   ##
==========================================
+ Coverage   44.16%   44.21%   +0.05%     
==========================================
  Files          89       89              
  Lines       11925    11925              
==========================================
+ Hits         5267     5273       +6     
+ Misses       6223     6216       -7     
- Partials      435      436       +1     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
| reqURL := c.url() | ||
| reqURL.Path = fmt.Sprintf("/api/v1/applications/%s", name) | ||
|  | ||
| req, err := http.NewRequest(http.MethodDelete, reqURL.String(), nil) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to set proper content-type for this request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added code to set the Content-Type header to application/json in a new commit
        
          
                .github/workflows/ci.yaml
              
                Outdated
          
        
      | - name: Install ArgoCD CRDs | ||
| run: kubectl apply -f https://raw.githubusercontent.com/argoproj/argo-cd/stable/manifests/crds.yaml | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually part of setting up the test environment later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! You were right, that part was related to the test environment setup.
Based on your comment, I've reverted the changes to ci.yaml.
I've modified the test setup logic instead of changing the CI configuration. The test suite now checks for the existence of the required contexts at startup. If a context is not found (as is common in a local environment), the test is now skipped with an informative message instead of failing.
This change has no impact on the CI environment, where the contexts are present and the tests will run as they always have. It simply makes the test suite more robust and convenient for local development.
1de9807    to
    8536769      
    Compare
  
    | break | ||
| } | ||
| } | ||
| asserts.True(found, "application %s not found in list", appName) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message is not correct
        
          
                test/e2e/application_test.go
              
                Outdated
          
        
      | // 5. Verify Deletion | ||
| _, err = s.argoClient.GetApplication(appName) | ||
| asserts.Error(err) | ||
| asserts.Contains(err.Error(), "not found") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this step is unnecessary.
| @jannfis The test case failure was caused by a nil issue. Since the test turned out to be unnecessary, I have removed it. Could you please approve the actions? | 
- set content type for delete request header - add create, list, get, delete api test Signed-off-by: yeonsoo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @juanxiu! Great job.
LGTM.
What does this PR do / why we need it:
This PR introduces a new end-to-end test case to validate the lifecycle of an Application resource by interacting directly with the Argo CD API.
Our existing end-to-end tests create Application resources declaratively, by applying manifests directly to the cluster. This approach has a gap: it doesn't fully simulate user actions performed through the Argo CD API. This can lead to subtle bugs in the API interaction path going undetected.
To improve test coverage and prevent similar issues, this change adds a test that specifically covers the API-driven creation, retrieval, and deletion of applications, ensuring our integration with the Argo CD API is robust.
Which issue(s) this PR fixes:
Fixes #413
How to test changes / Special notes to the reviewer:
CreateApplicationGetApplicationandListApplicationsDeleteApplicationThe client now correctly encapsulates the logic for authentication and request signing, making it a reliable tool for our E2E test suite.
This new test provides greater confidence that creating and managing applications via the Argo CD API works as expected in an environment where the agent is active.
Checklist