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

smarter action shuffler #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pbruski
Copy link

@pbruski pbruski commented Jan 28, 2020

No description provided.

@pbruski pbruski force-pushed the smarter-action-shuffler branch from 069f414 to a0db740 Compare January 28, 2020 10:50
@dagguh dagguh requested a review from a team February 12, 2020 15:58
@pbruski pbruski requested a review from dagguh February 13, 2020 13:32
import com.atlassian.performance.tools.jirasoftwareactions.api.actions.BrowseBoardsAction
import kotlin.reflect.KClass

class ActionShuffler {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not part of the API, so it could be internal, not to expose public non-API classes (it can be used by accident and expected to be compatible).

@wyrzyk
Copy link
Contributor

wyrzyk commented Feb 17, 2020

@pbruski Why do we need a smarter shuffler? What problem does it solve?

AFAIK the current one will skip some actions at the beginning, but it shouldn't be a problem (skipping is cheap and fast). On the other hand, the smarter one does assumptions on the scenario and introduces extra complexity. Are there other advantages of the smarter shuffler, that would justify introduced complexity?

issueKeyMemoriser: Action): List<Action> {
//createIssue needs a project - browserProject goes first
//viewIssue needs to have issues - createIssues goes second
//viewBoards needs to know about boards
Copy link
Contributor

Choose a reason for hiding this comment

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

A unit test would be a better way to explain and protect such contracts.

BTW this function could be internal too.

@@ -45,6 +45,8 @@ class JiraSoftwareScenario : Scenario {
jqlMemory = jqlMemory,
issueKeyMemory = issueKeyMemory
)
val findInitialIssues = ActionShuffler.findIssueKeysWithJql(jira, meter, issueKeyMemory)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it violates SRP. It doesn't seem to be the responsibility of shuffler to findIssueKeysWithJql.

import com.atlassian.performance.tools.jirasoftwareactions.api.actions.BrowseBoardsAction
import kotlin.reflect.KClass

class ActionShuffler {
Copy link
Contributor

Choose a reason for hiding this comment

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

The action is so tightly coupled with the scenario, that I would consider making it a private class. It doesn't feet to be reused even in the internal scope.

@wyrzyk wyrzyk requested a review from a team February 17, 2020 09:40
@dagguh
Copy link
Contributor

dagguh commented Feb 18, 2020

Related: atlassian/jira-actions#39

@pbruski pbruski force-pushed the smarter-action-shuffler branch from a0db740 to af821af Compare March 4, 2020 15:19
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.

3 participants