Skip to content

Conversation

@flowejam
Copy link

@flowejam flowejam commented Aug 6, 2024

Also added comments re. failed tests for repository._transform.
This is for #384

Also added comments re. failed tests for repository._transform.
Copy link
Member

@suhaibmujahid suhaibmujahid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution, @flowejam!

The tests are failing! Could you please fix that?

@flowejam
Copy link
Author

flowejam commented Aug 7, 2024

Thank you for your contribution, @flowejam!

The tests are failing! Could you please fix that?

Thanks @suhaibmujahid !

Apologies if I'm missing something here, but from the log it looks like the only test that is failing is the one I added - I've included a snippet below. I think there could be an issue with the way tests are detected?

  # The test currently fails here with an error - might indicate an issue with
        # the is_test function in bugbug/repository.py.
        repository.transform(hg, local, commit3)
>       assert commit3.test_files_modified_num == 1
E       AssertionError: assert 0 == 1

E.g. for bugbug, tests are in a folder called 'tests' in the main directory of the repo, but wouldn't they be missed by this function (repository.is_test)?:

# This code was adapted from https://github.com/mozsearch/mozsearch/blob/2e24a308bf66b4c149683bfeb4ceeea3b250009a/router/router.py#L127
def is_test(path: str) -> bool:
    return (
        "/test/" in path
        or "/tests/" in path
        or "/mochitest/" in path
        or "/unit/" in path
        or "/gtest/" in path
        or "testing/" in path
        or "/jsapi-tests/" in path
        or "/reftests/" in path
        or "/reftest/" in path
        or "/crashtests/" in path
        or "/crashtest/" in path
        or "/gtests/" in path
        or "/googletest/" in path
    )

@flowejam
Copy link
Author

@suhaibmujahid @marco-c just thought I'd follow up - any chance of getting another look at this?

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