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.

Sign up for GitHub

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

Jump to bottom

[TEST]: Improvement: Switches: New interaction approach: Phase3 #5763

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

[TEST]: Improvement: Switches: New interaction approach: Phase3 #5763

wants to merge 1 commit into from

Conversation

yuliiamir
Copy link
Collaborator

No description provided.

@yuliiamir yuliiamir force-pushed the test/improvement-switch-interaction branch 2 times, most recently from dfbaf49 to cd9c42b Compare January 27, 2025 10:38
@yuliiamir yuliiamir force-pushed the test/improvement-switch-interaction-phase3 branch from 241d75e to ff068b9 Compare January 27, 2025 14:45
@yuliiamir yuliiamir force-pushed the test/improvement-switch-interaction branch from cd9c42b to ce481bb Compare January 29, 2025 13:42
@yuliiamir yuliiamir force-pushed the test/improvement-switch-interaction-phase3 branch 4 times, most recently from 091834d to 1ad697a Compare February 4, 2025 09:45
@yuliiamir yuliiamir changed the base branch from test/improvement-switch-interaction to develop February 4, 2025 09:45
@yuliiamir yuliiamir force-pushed the test/improvement-switch-interaction-phase3 branch from 1ad697a to 44e778c Compare February 7, 2025 11:46
IvanChupin
IvanChupin approved these changes Feb 13, 2025
View reviewed changes
...onal-tests/src/main/groovy/org/openkilda/functionaltests/helpers/model/SwitchExtended.groovy Outdated
@@ -210,6 +212,11 @@ class SwitchExtended {
return new PortExtended(sw, portNo, northbound, northboundV2, cleanupManager)
}

PortExtended getPortInState(String state) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider to use enum constants here

...ests/src/test/groovy/org/openkilda/functionaltests/spec/switches/SwitchPropertiesSpec.groovy Outdated

def setupSpec() {
switchUnderTest = switches.all().first()
def initialProps = switchUnderTest.getCashedProps()
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo, the name of getCashedProps method. should be getCachedProps.

also worth to mention, the following is out of this PR scope, but:
the result of getCashedProps method is cached with @memoized.
I can see many usage of this caching. How cache eviction mechanism is implemented here? what if cache becomes outdated?

...ests/src/test/groovy/org/openkilda/functionaltests/spec/switches/SwitchPropertiesSpec.groovy Outdated
FlowEncapsulationType.VXLAN.toString().toLowerCase()].sort() :
[FlowEncapsulationType.VXLAN.toString().toLowerCase()]
switchProperties.tap {
[TRANSIT_VLAN.toString(), VXLAN.toString().toLowerCase()].sort() : [VXLAN.toString().toLowerCase()]
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't you need to call toLowerCase for TRANSIT_VLAN as well?

Copy link
Collaborator Author

@yuliiamir yuliiamir Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nope, used org.openkilda.functionaltests.helpers.model.FlowEncapsulationType instead of org.openkilda.model.FlowEncapsulationType (will update the rest of encaps)

...ests/src/test/groovy/org/openkilda/functionaltests/spec/switches/SwitchPropertiesSpec.groovy
with(northboundV2.getAllSwitchProperties().switchProperties.find { it.switchId == sw.dpId }){
supportedTransitEncapsulation.sort() == newTransitEncapsulation
}
sw.getProps().supportedTransitEncapsulation.sort() == newTransitEncapsulation
Copy link
Collaborator

Choose a reason for hiding this comment

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

out of PR scope, but

if sw.getPropsV1 calls northbound v1 API, and sw.getProps calls northbound v2 API, it might be worth to rename getProps() method to getPropsV2().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mostly used v2 calls and for the v1 calls we just specify v1

...functional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/model/Switches.groovy Outdated
@@ -77,6 +79,10 @@ class Switches {
switches.findAll().unique { it.hwSwString()}
}

List<SwitchExtended> notOF12Version() {
switches.findAll { it.ofVersion != "OF_12" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be out of this pr scope, but consider to use constant here and in other places for "OF_12" string

@yuliiamir yuliiamir force-pushed the test/improvement-switch-interaction-phase3 branch 2 times, most recently from 30b6413 to 0a83ba4 Compare February 13, 2025 16:12
@yuliiamir yuliiamir force-pushed the test/improvement-switch-interaction-phase3 branch from 0a83ba4 to a2c9c28 Compare February 14, 2025 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IvanChupin IvanChupin IvanChupin approved these changes

@pablomuri pablomuri pablomuri approved these changes

Assignees
No one assigned
Labels
area/testing improvement On Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuliiamir @IvanChupin @pablomuri