Naming conventions for automated tests of TC components



2 years ago
2 years ago


(Reporter: pmoore, Unassigned)





2 years ago
Let's agree some conventions across all projects for automated tests. Having naming conventions will help us keep our house clean. =)

I've been inconsistent so far, I'd like to clean this up, so at the same time maybe we can align on some conventions. This is what I have been using so far...

Currently used names *** BAD ***



  clientId:      "travis_generic-worker"
  role:          "client-id:travis_generic-worker"
  provisionerId: "test-provisioner"
  schedulerId:   "test-scheduler"
  workerGroup:   "test-worker-group"
  workerId:      "test-worker-id"
  workerType:    slugid.Nice()



  clientId:      "travis_tc-client-go"
  role:          "client-id:travis_tc-client-go"
  provisionerId: "win-provisioner"
  schedulerId:   "go-test-test-scheduler"
  workerType:    "win2008-worker"



  clientId:      "travis_tc-client-go" (whoops)
  role:          "client-id:travis_tc-client-go" (whoops)
  provisionerId: "win-provisioner"
  schedulerId:   "junit-test-scheduler"
  workerType:    "win2008-worker"

Other components

I've only listed the projects I've written the tests for, but we also have tests for e.g.

  * docker-worker
  * queue
  * auth
  * scheduler
  * big graph scheduler
  * hooks
  * taskcluster proxy
  * relengapi proxy

As we have so many projects using various provisionerIds, roles, clientIds, schedulerIds etc, it would make sense to align for bookkeeping purposes.

Naming convention suggestions

For worker tests, it makes sense for `workerType` to be a slug, so that parallel tests don't try to pop the same test tasks off the Azure task queues. Note, the client libraries don't test claiming tasks, so fixed strings can be used for workerTypes there without risk of runtime test collision.

I like the idea of "[repo]:[branch]:unittest", e.g.

  * "docker-worker:master:unittest"
  * "taskcluster-client-go:bug1234567:unittest" 

It could be argued that this information would be more appropriate for the `taskdefinitionrequest.metadata.source` field. What do you guys think?

I prefer a more explicit name for the provisioner than "dummy-garbage", such as "fake-provisioner-for-taskcluster-unittests", since somebody stumbling on a reference to this provisioner would understand its purpose.

How about "[ci-system]:[repo]", e.g.

  * "travis:generic-worker"
  * "circleci:docker-worker"

How about "unittest:[repo]", e.g.

  * "unittest:generic-worker"
  * ""

These seem the most important fields relating to workers - we might want to consider conventions for other fields too.

Comment 1

2 years ago
NI'ing affected people...
Flags: needinfo?(jopsen)
Flags: needinfo?(jhford)
Flags: needinfo?(garndt)
Flags: needinfo?(dustin)
I'd rather have the constant string as a prefix.  Also, these aren't unit tests.


etc.  All of the identifiers should start that way.
Flags: needinfo?(dustin)

Comment 3

2 years ago
(In reply to Dustin J. Mitchell [:dustin] from comment #2)
> I'd rather have the constant string as a prefix.  Also, these aren't unit
> tests.
>   tests:[repo]:[branch]
> etc.  All of the identifiers should start that way.


Comment 4

2 years ago
I definitely agree with a constant string for a prefix and also echo the need for some uniqueness as described in Pete's suggestion for workertype. We do this in docker-worker, although the prefix is stupidly named "dummy-type" and "dummy-worker".  We append a slugid to give some uniqueness and trim to 22 characters.  Definitely isn't great, but so far has worked.  Scheduler ID is static with "docker-worker-tests" and provisioner is "no-provisioning-nope" (I'm not sure where that originally came from).  

However, having something like "tests:[repo]:[branch]" would put us over the edge for identifier-max-length.  "tests:docker-worker:master" is 26 characters.
Flags: needinfo?(garndt)


2 years ago
See Also: → bug 1228284

Comment 5

2 years ago
Thanks for the feedback - I propose we go ahead and start making the changes.

One extra thing is we should probably document the naming conventions somewhere, is a sensible place? Normally this is for consumers of taskcluster, but it is our canonical documentation source - so maybe it is still ok.
Flags: needinfo?(jopsen)
Flags: needinfo?(jhford)
That should go in devel/conventions or somewhere else under devel/

Comment 7

2 years ago
I'll raise a PR...
@pmoore, sorry for late feedback.

But: schedulerId, provisionerId, workerType, workerId, workerGroup
Are all limited to 22 characters a-z0-9_-
 - otherwise we couldn't fit all of them in RabbitMQ routing keys :)
(generally, this limitation has worked reasonably well)

Due to this limitation, I started moving towards using "dummy-garbage-*".
So there is room for 8 random characters. 
I hope we can agree that a name "dummy-garbage-" should scare people from using it in production :)

IMO we could just do:

1) Decide on a list of dummy scopes that can be given to all test suites.
   (Eases configuration, but all tests suites will get the same scopes, seems acceptable)
2) Create a role s.t: assume:role-id:taskcluster-developer -> [ list of dummy scopes ]
3) For each ldap group that should able to run unit tests, grant them the scope:
4) Create a role s.t: assume:client-id:tc-ci/* -> [assume:role-id:taskcluster-developer]
5) For each travis/circle-ci file, create a clientId:  tc-ci/<component>
   (no need to create a role, as the one from (4) will just work)

Downside of (1) is that all the dummy scopes will be given to every test. Ie. client library tests
might not need scopes to claim task. But since they are all dummy scopes, what is the point of locking
things down and creating lots of roles, just to protect a few workerTypes only used for tests anyways.

Anyway reason this won't work? Or use-cases this won't cover?

We can document how this works in the "description" field for:
  assume:role-id:taskcluster-developer (documents why these scopes are dummy scopes)
  assume:client-id:tc-ci/* (documents how credentials for travis is created)
Note: I'm not even sure we need external docs, because the "assume:role-id:taskcluster-developer"
documents what is dummy scopes. And tests only gets the dummy scopes. The process for creating a
new dummy scope is reaching consensus with involved parties and adding it to the
assume:role-id:taskcluster-developer role. 

@pmoore, I think (1) through (5) is a bit simpler and reduces configuration pains, while providing
decent security. Thoughts?
Flags: needinfo?(pmoore)

Comment 9

2 years ago
Yep, yet again I totally agree - your razor sharp wisdom prevails. Thanks for halting the train on this one - your suggestion is much better. Let's go with that.
Flags: needinfo?(pmoore)
You need to log in before you can comment on or make changes to this bug.