jina-ai / executors

internal-only
Apache License 2.0
31 stars 12 forks source link

ci: notice changes in test folders #150

Closed jakobkruse1 closed 3 years ago

jakobkruse1 commented 3 years ago

This branch fixes the bug that the CI does not detect changes in the test folders

tadejsv commented 3 years ago

I think we need to change the approach completely, and explicitly enumerate the allowed folders. Otherwise, there are too many cases to consider:

jakobkruse1 commented 3 years ago

I think we need to change the approach completely, and explicitly enumerate the allowed folders. Otherwise, there are too many cases to consider:

* what if it's a change in the `tests/integration`?

* what if it's a change in `executor/some_module/some_other_module`

* etc.

How would you suggest we do that?

cristianmtr commented 3 years ago

I think we need to change the approach completely, and explicitly enumerate the allowed folders. Otherwise, there are too many cases to consider:

  • what if it's a change in the tests/integration?

integration tests are always run

  • what if it's a change in executor/some_module/some_other_module

Good point. Maybe outside the scope of this PR, but I'll open an issue to track it

tadejsv commented 3 years ago

I think we need to change the approach completely, and explicitly enumerate the allowed folders. Otherwise, there are too many cases to consider:

* what if it's a change in the `tests/integration`?

* what if it's a change in `executor/some_module/some_other_module`

* etc.

How would you suggest we do that?

My idea is something like this: list all the folders where executor directories can be, so this would be

There should only be a small number of them - and they should not change very much over time.

Here you list the final folder, right before the executor one. And then write a script that goes over these folders and collects the names of all the executor folders within them.

And then, in the part of the script that goes over changed files, check if any of these folders were matched.

Since this can be a pain to implement in bash, I would recommend doing it in python.

tadejsv commented 3 years ago

I think we need to change the approach completely, and explicitly enumerate the allowed folders. Otherwise, there are too many cases to consider:

  • what if it's a change in the tests/integration?

integration tests are always run

These are the executor integration tests

cristianmtr commented 3 years ago

I think we need to change the approach completely, and explicitly enumerate the allowed folders. Otherwise, there are too many cases to consider:

* what if it's a change in the `tests/integration`?

* what if it's a change in `executor/some_module/some_other_module`

* etc.

How would you suggest we do that?

My idea is something like this: list all the folders where executor directories can be, so this would be

  • jinahub/encoders/{text,audio,video}
  • jinahub/rankers
  • jinahub/segmenters
  • ...

There should only be a small number of them - and they should not change very much over time.

Here you list the final folder, right before the executor one. And then write a script that goes over these folders and collects the names of all the executor folders within them.

And then, in the part of the script that goes over changed files, check if any of these folders were matched.

Since this can be a pain to implement in bash, I would recommend doing it in python.

I think a simpler solution would be to keep going up until the the folder has a tests folder. That's what we care about in the end, right?

jakobkruse1 commented 3 years ago

I think a simpler solution would be to keep going up until the the folder has a tests folder. That's what we care about in the end, right?

This could be possible yes. We can just go up until there is a test folder -> then run CI or until we are in the root of the repository, then continue with the next changed file