Skip to content

Test worker priorization#45

Open
Lorenaps wants to merge 3 commits intomasterfrom
test-worker-priorization
Open

Test worker priorization#45
Lorenaps wants to merge 3 commits intomasterfrom
test-worker-priorization

Conversation

@Lorenaps
Copy link
Copy Markdown
Contributor

@Lorenaps Lorenaps commented Apr 14, 2021

Para poder pensar nas 2 funções getPipelinesThatFailed e getPipelinesForCompleteHistory precisei pensar no processo como um todo a fim de deixar essas funções "mais simples", e então centralizar as regras que queremos aplicar no prioritizeAndLimit. Por isso fiz essa primeira versão de teste, o que me ajudou a pensar nas funções novas no storage também =].

O próximo passo é implementar as funções no storage para depois fazer o getPipelinesThatFailed.
E acredito que esse processo deve gerar algum ajuste nas ideias descritas aqui para a priorização.

@Lorenaps Lorenaps requested a review from danielfireman April 14, 2021 00:29
Comment thread worker/worker.go
type dbInterface interface {
GetPipelinesByDay(day int) ([]storage.Pipeline, error)
InsertExecution(e storage.Execution) error
GetLastExecutionsForAllPipelines() ([]storage.Execution, error)
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.

Você provavelmente vai precisar passar algum tipo de quantidade aqui, não?

Outra coisa importante, a interface pode ser um bom lugar para documentar bem essas funções. Por exemplo, o método GetPipelinesByDay(int) retorna todos os pipelines do dia especificado, sempre no mês corrente.

Comment thread worker/worker.go
log.Fatal("error trying get environment variable: $MONGODB is empty")
}

errorLimitStr := os.Getenv("ERROR_LIMIT")
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.

ERROR_LIMIT é a quantidade de vezes que a execução de um determinado mês pode falhar? Se sim, poderia chamar ela RETRY_LIMIT, por favor? Essa é a forma que ela se apresenta em outros programas.

Também poderia comentar essa variável? Vale super pena documentar esse parâmetro do programa.

Comment thread worker/worker.go
}

errorLimitStr := os.Getenv("ERROR_LIMIT")
if uri == "" {
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.

Suggested change
if uri == "" {
if errorLimitStr == "" {

Comment thread worker/worker.go
}

errorLimitStr := os.Getenv("ERROR_LIMIT")
if uri == "" {
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.

Lembrando que o strconv.Atoi falha quando a variável é vazia. Mas entendo que você quis dar uma mensagem mais bacana quando o erro mais recorrente.

Comment thread worker/worker.go
}

func getPipelinesToExecuteToday(db *storage.DBClient) ([]storage.Pipeline, error) {
func getPipelinesToExecuteToday(db dbInterface) ([]storage.Pipeline, error) {
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.

Eu acho arretada essa forma que programas em Go crescem em complexidade. Hoje em dia me soa bastante orgânico, natural? Nenhuma modição em extends ou implements! Note que você não precisou modificar em nada storage.DBClient!

Comment thread worker/worker_test.go
},
}

type fakeFinder struct {
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.

fakeDB?

Comment thread worker/worker_test.go
"github.com/matryer/is"
)

var pipelinesDB = []storage.Pipeline{
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.

Tu deve precisar criar vários desses a medida que o teste aumenta.

Sugestão: cria os storage.Pipeline baseado nas características que você quer, coloca nomes sugestivos neles (dada as características de erro ou algo simples ou complexo ou algo assim) e coloca eles na parte de baixo do programa.

Daí, cria o []storage.Pipeline dentro de cada método. A pessoa lendo esse arquivo ver os casos de teste e quando for ler um terminado método vai entender mais fácilmente o caso sendo testado.

Comment thread worker/worker_test.go
// 2º descartar se o número de execuções com erro for igual ou maior que o limite
// ...
// por último aplicar filtro de tamanho
func TestPrioritizeAndLimit_LimitTest(t *testing.T) {
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.

Isso está me parecendo um excelente caso para table driven tests (aqui e aqui).

cada caso de teste vai receber uma lista (pipeline), um limite a ser passaddo para função e o valor experado.

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.

2 participants