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

e2e: make running subsets of e2e tests more organized #1512

Merged
merged 4 commits into from
Sep 22, 2023
Merged

e2e: make running subsets of e2e tests more organized #1512

merged 4 commits into from
Sep 22, 2023

Conversation

hj-johannes-lee
Copy link
Contributor

@hj-johannes-lee hj-johannes-lee commented Aug 29, 2023

closes: #1143

@hj-johannes-lee hj-johannes-lee marked this pull request as draft August 29, 2023 10:21
@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2023

Codecov Report

Merging #1512 (6dd7717) into main (61dc798) will decrease coverage by 0.07%.
Report is 5 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 6dd7717 differs from pull request most recent head 5347665. Consider uploading reports for the commit 5347665 to get more accurate results

@@            Coverage Diff             @@
##             main    #1512      +/-   ##
==========================================
- Coverage   49.54%   49.48%   -0.07%     
==========================================
  Files          42       42              
  Lines        4923     4923              
==========================================
- Hits         2439     2436       -3     
- Misses       2343     2345       +2     
- Partials      141      142       +1     
Files Changed Coverage Δ
cmd/qat_plugin/dpdkdrv/dpdkdrv.go 83.27% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mythi
Copy link
Contributor

mythi commented Aug 30, 2023

I realize it's still WIP but two quick comments: 1) DEVEL.md has a place for e2e tests, 2) could we stop using "demo" and say, e.g., "app".

@hj-johannes-lee
Copy link
Contributor Author

hj-johannes-lee commented Aug 31, 2023

@mythi Thank you for your comment! :)

  1. I know there is some parts in DEVEL.md, and actually wanted to ask about this. Do you want me to add the readme to there, or can I remove that part from DEVEL.md (and move some parts to readme)? I think we need to explain more things than before.. So, Idk if DEVEL.md would be a good place.

  2. What do you think about some pods that run just busybox and checks the availability of the resources? What would be a good name for those? Just a pod?

@mythi
Copy link
Contributor

mythi commented Aug 31, 2023

  1. know there is some parts in DEVEL.md, and actually wanted to ask about this. Do you want me to add the readme to there, or can I remove that part from DEVEL.md (and move some parts to readme)? I think we need to explain more things than before.. So, Idk if DEVEL.md would be a good place.

DEVEL.md is preferred because we have tried to move documentation in as few places as possible and so that we have docs for "workload owners", "cluster admins", and "developers". Not quite there yet but let's not add new places at least.

@hj-johannes-lee hj-johannes-lee changed the title e2e: make it possible to run subsets of e2e tests. e2e: make it orginazed to run subsets of e2e tests. Sep 4, 2023
@hj-johannes-lee hj-johannes-lee changed the title e2e: make it orginazed to run subsets of e2e tests. e2e: make it more organized to run subsets of e2e tests. Sep 4, 2023
@hj-johannes-lee hj-johannes-lee changed the title e2e: make it more organized to run subsets of e2e tests. e2e: make it more organized to run subsets of e2e tests Sep 4, 2023
@hj-johannes-lee hj-johannes-lee changed the title e2e: make it more organized to run subsets of e2e tests e2e: make running subsets of e2e tests more organized Sep 4, 2023
@hj-johannes-lee hj-johannes-lee marked this pull request as ready for review September 4, 2023 21:02
@hj-johannes-lee hj-johannes-lee marked this pull request as draft September 4, 2023 21:02
@hj-johannes-lee hj-johannes-lee marked this pull request as ready for review September 4, 2023 21:10
@hj-johannes-lee
Copy link
Contributor Author

About the matter of changing everything that has 'demo' to 'app'.. I thought it would take long time to change everything, and it is not actually related to this PR that much, so I just didn't touch anything. Just changed the label from "Demo" to "App".

In addition, I also wrote the further instruction of using labels and running subsets of tests in DEVEL.md.

It seems that CI fails, but don't think it is related to this PR?

DEVEL.md Outdated Show resolved Hide resolved
DEVEL.md Outdated Show resolved Hide resolved
@mythi
Copy link
Contributor

mythi commented Sep 5, 2023

Just changed the label from "Demo" to "App".

That was my ask, so good that you did not do anything else

Copy link
Contributor

@mythi mythi left a comment

Choose a reason for hiding this comment

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

looking good to me. some questions/comments

DEVEL.md Show resolved Hide resolved
test/e2e/dlb/dlb.go Outdated Show resolved Hide resolved
DEVEL.md Outdated Show resolved Hide resolved
@@ -39,7 +39,7 @@ const (
)

func init() {
ginkgo.Describe("DLB plugin", describe)
ginkgo.Describe("DLB plugin [Device:dlb]", describe)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a good suggestion but it looks that now these node descriptions have a lot of repeating words. Since each description is modified by this PR, I was wondering if it made sense to reword them a bit. How does the output look for a device when all tests are run?

Copy link
Contributor Author

@hj-johannes-lee hj-johannes-lee Sep 20, 2023

Choose a reason for hiding this comment

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

Well, I also thought about this matter, and have been thinking for a long time after you wrote this comment, but I could not think either.
I took a look e2e tests in Kubernetes, but they also repeat in many cases.
e.g.

  • ginkgo.It("should increase cluster size if pending pods are small [Feature:ClusterSizeAutoscalingScaleUp]", func(ctx context.Context){})
  • ginkgo.It("shouldn't increase cluster size if pending pod is too large [Feature:ClusterSizeAutoscalingScaleUp]", func(ctx context.Context) {})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, in our case, we have many layers so.. it would look dirtier though..

Copy link
Contributor Author

@hj-johannes-lee hj-johannes-lee Sep 20, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

We can keep it as it is but I feel it could be optimized. Like: how many times QAT is mentioned on that row? Or, would ginkgo.Describe("[Device:dlb] plugin", describe) be any worse? Just thinking out loud.

Copy link
Contributor Author

@hj-johannes-lee hj-johannes-lee Sep 20, 2023

Choose a reason for hiding this comment

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

Like: how many times QAT is mentioned on that row?

Umm, that is not related to this PR..! They are already mentioned multiple times without the labels.. :(
Do you want me to refine that in general?

ginkgo.Describe("[Device:dlb] plugin", describe)

Actually it was what I was thinking at first,, but then have no idea for [Resource] or [App] because some verbs (e.g. 'deploys') or 'When' should be before the labels...

For example,

Describe("[Device:qat] plugin in [Mode:dpdk")
  Context("When [Resource:generic] are available")
    It("deploys [App:compress-perf] requesting QAT resources")

To me,, this looked more messy because labels seem like to be in random places...

Copy link
Contributor

Choose a reason for hiding this comment

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

Umm, that is not related to this PR..! They are already mentioned multiple times without the labels.. :(

My original comment was just "something for you to think about". It's not directly related to this PR but here we are modifying these lines so there could be an opportunity to have refinements included in the same changes.

Copy link
Contributor

@mythi mythi left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks a lot for the efforts! A couple of small comments. If QAT kernel is turning problematic, we can just drop it...

Makefile Outdated Show resolved Hide resolved
DEVEL.md Show resolved Hide resolved
DEVEL.md Outdated Show resolved Hide resolved
Strictly speaking, 'Gen4' and 'Gen2' are wrong expressions in this case,
because Gen4 resources are read as 'generic' in VMs. To prevent any
confusion, use just the names of the QAT services such as 'dc', 'cy' or
'generic'

Signed-off-by: Hyeongju Johannes Lee <[email protected]>
Signed-off-by: Hyeongju Johannes Lee <[email protected]>
Copy link
Contributor

@mythi mythi left a comment

Choose a reason for hiding this comment

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

nice improvement, thanks!

@mythi mythi merged commit bcf8f01 into intel:main Sep 22, 2023
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

restructure and document e2e "focus" targets
4 participants