Skip to content

smarter action shuffler#6

Open
pbruski wants to merge 2 commits into
atlassian:masterfrom
pbruski:smarter-action-shuffler
Open

smarter action shuffler#6
pbruski wants to merge 2 commits into
atlassian:masterfrom
pbruski:smarter-action-shuffler

Conversation

@pbruski
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

jqlMemory = jqlMemory,
issueKeyMemory = issueKeyMemory
)
val findInitialIssues = ActionShuffler.findIssueKeysWithJql(jira, meter, issueKeyMemory)
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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