Organize specs for pipeline creation

This issue is to foster some discussion around pain points related to pipeline creation specs.

Problem

Today we have ~2000 LoC for Ci::CreatePipelineService specs and ~2000 LoC for Gitlab::Ci::YamlProcessor specs + a lot of other other specific unit tests.

  • It's difficult to navigate the specs give their size
  • Having very long context blocks causes to have setup very far away from the test case and this brings other set of disadvantages
  • Hard to have a clear picture of the behavior involved
  • Difficult to choose at what level we should test things, especially for new team members

Thoughts

  • Testing each Pipeline::Chain::*** class in isolation does not make sense because it strictly depends on the earlier steps of the chain. We should prefer to add specs to Ci::CreatePipelineService to test the logic end-to-end.

  • We need to define clear testing practices for common scenarios: e.g. wen we add support for a new keyword in .gitlab-ci.yml. We tend to add specs to YamlProcessor as well as CreatePipelineService. I think we should prefer CreatePipelineService over YamlProcessor as the latter is only 1 of the early steps of the pipeline creation chain. Results could be different with the end-to-end test. This defines the intention that we treat CreatePipelineService as black-box service.

  • We need to come up with a better organization of specs around CreatePipelineService as it can grow exponentially. We could organize specs for create_pipeline_service into separate files each implementing cohesive test cases. We could also implement an helper module to be included in these spec classes to take advantage of common behavior.

spec/services/ci/create_pipeline_service_spec.rb # only high level behavior implemented directly in `create_pipeline_service.rb`
spec/services/ci/create_pipeline_service/
  |- config_specs.rb
  |- skip_spec.rb
  |- environment_spec.rb
  |- protected_refs_spec.rb
  |- workflow_rules_spec.rb
  |- merge_requests_spec.rb
  |- external_pull_requests_spec.rb
  |- ...

Existing "E2E" specs that should be moved:

Edited by drew stachon