Skip to content

NETOBSERV-2113 CLI Integration tests#212

Merged
openshift-merge-bot[bot] merged 3 commits intonetobserv:mainfrom
memodi:integration-tests
Apr 8, 2025
Merged

NETOBSERV-2113 CLI Integration tests#212
openshift-merge-bot[bot] merged 3 commits intonetobserv:mainfrom
memodi:integration-tests

Conversation

@memodi
Copy link
Member

@memodi memodi commented Mar 6, 2025

Description

Integration tests for CLI

$ ginkgo run
Running Suite: IntegrationTests Suite - /Users/memodi/workspaces/repos/netobserv/network-observability-cli/e2e/integration-tests
================================================================================================================================
Random Seed: 1741229991

Will run 2 of 2 specs
••

Ran 2 of 2 Specs in 152.239 seconds
SUCCESS! -- 2 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS

Ginkgo ran 1 suite in 3m9.985017885s
Test Suite Passed

Dependencies

#201

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
    • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
    • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
    • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
    • Standard QE validation, with pre-merge tests unless stated otherwise.
    • Regression tests only (e.g. refactoring with no user-facing change).
    • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

@memodi memodi requested review from Amoghrd, jotak and jpinsonneau March 6, 2025 03:17
@codecov
Copy link

codecov bot commented Mar 13, 2025

Codecov Report

Attention: Patch coverage is 0% with 84 lines in your changes missing coverage. Please review.

Project coverage is 22.30%. Comparing base (b5a86ca) to head (f843dd1).

Files with missing lines Patch % Lines
e2e/integration-tests/cli.go 0.00% 46 Missing ⚠️
e2e/integration-tests/cluster.go 0.00% 38 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #212      +/-   ##
==========================================
- Coverage   23.70%   22.30%   -1.41%     
==========================================
  Files          11       13       +2     
  Lines        1333     1417      +84     
==========================================
  Hits          316      316              
- Misses       1000     1084      +84     
  Partials       17       17              
Flag Coverage Δ
unittests 22.30% <0.00%> (-1.41%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
e2e/integration-tests/cluster.go 0.00% <0.00%> (ø)
e2e/integration-tests/cli.go 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@memodi
Copy link
Member Author

memodi commented Mar 13, 2025

@jpinsonneau any idea why konflux pipelines prefetch-dependencies task is failing?

Failure snippet:
task prefetch-dependencies has the status "Failed":
Please try running `go mod vendor` and committing the changes.
  Note that you may need to `git add --force` ignored files in the vendor/ dir.
  Docs: https://github.com/containerbuildsystem/cachi2/blob/main/docs/gomod.md#vendoring

@jpinsonneau
Copy link
Member

@jpinsonneau any idea why konflux pipelines prefetch-dependencies task is failing?

Failure snippet:
task prefetch-dependencies has the status "Failed":
Please try running `go mod vendor` and committing the changes.
  Note that you may need to `git add --force` ignored files in the vendor/ dir.
  Docs: https://github.com/containerbuildsystem/cachi2/blob/main/docs/gomod.md#vendoring

Good question. The go mod tidy / vendor commands doesn't update any file so I think we are good here.
Let's retest first as Konflux is quite flaky

@jpinsonneau
Copy link
Member

/retest

@jpinsonneau
Copy link
Member

@OlivierCazade could you please have a look to the konflux error ? 👼

@memodi
Copy link
Member Author

memodi commented Mar 17, 2025

/retest

@memodi memodi force-pushed the integration-tests branch from 9093862 to b9eda11 Compare March 17, 2025 18:39
@OlivierCazade
Copy link
Member

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented Mar 18, 2025

New changes are detected. LGTM label has been removed.

@Amoghrd
Copy link
Member

Amoghrd commented Mar 20, 2025

LGTM

Comment on lines +66 to +72
var flow Flowlog
for decoder.More() {
err := decoder.Decode(&flow)
o.Expect(err).NotTo(o.HaveOccurred())
if flow.SrcK8sNamespace != nsfilter {
o.Expect(true).To(o.BeFalse(), fmt.Sprintf("Flow %v does not meet regexes condition SrcK8S_Namespace~%s", flow, nsfilter))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we should decode using a map[string)interface{}) here to avoid keeping the FlowLog definition in the tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will also reduce the dependencies in the go.mod file

Copy link
Member Author

@memodi memodi Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we use map[string]interface{} then we'd need to do type assertions when we read those fields, correct? that make code bit untidy especially when we need to read in nested structures. wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as an alternative, you can declare the type inline with just what you need, such as:

		var flow struct {
			SrcK8sNamespace string `json:"SrcK8S_Namespace"`
		}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, that's neat!

@memodi
Copy link
Member Author

memodi commented Apr 7, 2025

@jpinsonneau @jotak - any concerns on getting this merged?

@openshift-ci
Copy link

openshift-ci bot commented Apr 8, 2025

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jpinsonneau
Copy link
Member

@jpinsonneau @jotak - any concerns on getting this merged?

I'm merging this now. Thanks @memodi !

@memodi
Copy link
Member Author

memodi commented Apr 8, 2025

/retest

@openshift-merge-bot openshift-merge-bot bot merged commit 2f92f14 into netobserv:main Apr 8, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants