Refactor projects fixtures

[tanvimoharir]

Refers #9332

[benjaoming]

Hi @tanvimoharir - thanks for this! Your work looks great 👏

• I think you can address test failures like this:
  • All Git repos should have https (some have http!)
  • Merge in the latest main branch
• Once you think it's ready or you have no more ideas to move it forward, feel free to mark it ready for review 👍

[tanvimoharir]

Hi @tanvimoharir - thanks for this! Your work looks great 👏

• I think you can address test failures like this:

  • All Git repos should have https (some have http!)
  • Merge in the latest main branch

• Once you think it's ready or you have no more ideas to move it forward, feel free to mark it ready for review 👍

Thanks @benjaoming , I was working on the test failures. I will resolve them soon and mark this ready for review.

[tanvimoharir]

Hi @benjaoming , I have resolved all the test errors and some test failures. Currently the remaining test failures are mostly around version related test (and some with build as well) I saw that in some tests the expected version for pip project is 0.8 but with my fixtures script its version is latest. Should I be keeping any specific version for projects creation(in fixtures)
Let me know.
Thanks!

[benjaoming]

@tanvimoharir I would be happy to give this a review once the tests are working. Your approach seems to be the right one. Please do ping me if you're stuck with something.

[benjaoming]

For the tests that rely on specific Version objects to exist, we can add a minimal list of dictionaries for each project that has custom versions. Dictionary should contain some sort of minimal fixture data for the extra Version objects to create for the project.

Unfinished example:

       projects = [
            # ...
            ("Pip", "https://github.com/pypa/pip", [{"slug": "0.8", "identifier": "2404a34eba4ee9c48cc8bc4055b99a48354f4950"}]),
            # ...
        ]

[tanvimoharir]

@benjaoming I tried with name and slug (but not the identifier) and the error changed to 'Project matching query does not exist'. I will try with the exact dictionaries and see. Thanks!

[benjaoming]

Hi @tanvimoharir 👋

If you can make these changes work for exactly the same set of tests, that's perfect. That makes the change neutral.

After this, you have paved the way for everyone to benefit from simpler fixtures, meaning that we can trim/update test fixtures and add better test cases.

I saw that in some tests the expected version for pip project is 0.8 but with my fixtures script its version is latest.

True! There are tests checking if a specific build.Version is correct. A version can both pertain a specific build of a git commit or a git tag. The old fixtures contained some specific versions that are important for these particular tests to work.

These Version objects need to exist. I trimmed the original JSON to what I think should be necessary to specify for a "minimal" fixture model, which could simply be a dictionary: I ended up with just 2 essential fields: the identifier (git commit hash) + slug (version number). Everything else can be set by default or derived from the context (i.e. the Project is an invariant of the for loop, the most recently created project) and the verbose_name can be the same as slug.

[tanvimoharir]

Hi @benjaoming , I have been trying to update the test data according but have some doubts:
So I added something similar to get my tests passing:

In [31]: ProjectRelationship.objects.all()
Out[31]: <ChildRelatedProjectQuerySet []>

In [32]: Project.objects.get_or_create(
    ...:             name="Pip",
    ...:             slug="0.8",
    ...:             repo="https://github.com/pypa/pip",
    ...:             repo_type="git",
    ...:         )
[debug    ] Executing TaskRouter.          [readthedocs.builds.tasks] task=readthedocs.search.tasks.index_objects_to_es
[debug    ] Skipping routing non-build task. [readthedocs.builds.tasks] task=readthedocs.search.tasks.index_objects_to_es
Out[32]: (<Project: Pip>, True)

In [33]: Project.objects.all().values()
Out[33]: <ProjectQuerySetBase [{'id': 68, 'pub_date': datetime.datetime(2022, 10, 5, 13, 38, 1, 153337, tzinfo=<UTC>), 'modified_date': datetime.datetime(2022, 10, 5, 13, 38, 1, 153352, tzinfo=<UTC>), 'name': 'Pip', 'slug': '0.8', 'description': '', 'repo': 'https://github.com/pypa/pip', 'repo_type': 'git', 'project_url': '', 'canonical_url': '', 'single_version': False, 'default_version': 'latest', 'default_branch': None, 'requirements_file': None, 'documentation_type': 'sphinx', 'urlconf': None, 'external_builds_enabled': False, 'external_builds_privacy_level': 'public', 'cdn_enabled': False, 'analytics_code': None, 'analytics_disabled': False, 'container_image': None, 'container_mem_limit': None, 'container_time_limit': None, 'build_queue': None, 'max_concurrent_builds': None, 'allow_promos': True, 'ad_free': False, 'is_spam': None, 'show_version_warning': False, 'enable_epub_build': True, 'enable_pdf_build': True, 'path': '', 'conf_py_file': '', 'featured': False, 'skip': False, 'install_project': False, 'python_interpreter': 'python3', 'use_system_packages': False, 'privacy_level': 'public', 'language': 'en', 'programming_language': 'words', 'main_language_project_id': None, 'has_valid_webhook': False, 'has_valid_clone': False, 'remote_repository_id': None}]>

In [34]: Version.objects.all().values()
Out[34]: <VersionQuerySetBase [{'id': 61, 'created': datetime.datetime(2022, 10, 5, 13, 38, 1, 165765, tzinfo=<UTC>), 'modified': datetime.datetime(2022, 10, 5, 13, 38, 1, 165784, tzinfo=<UTC>), 'project_id': 68, 'type': 'branch', 'identifier': 'master', 'verbose_name': 'latest', 'slug': 'latest', 'supported': True, 'active': True, 'state': None, 'built': False, 'uploaded': False, 'privacy_level': 'public', 'hidden': False, 'machine': True, 'has_pdf': False, 'has_epub': False, 'has_htmlzip': False, 'documentation_type': 'sphinx'}]>

And then considering one of the tests which is actually failing

https://github.com/readthedocs/readthedocs.org/blob/main/readthedocs/rtd_tests/tests/test_api.py#L2367-L2368

Or

https://github.com/readthedocs/readthedocs.org/blob/main/readthedocs/rtd_tests/tests/test_api.py#L2471-L2472

And then another test which requires LATEST pip:

https://github.com/readthedocs/readthedocs.org/blob/main/readthedocs/rtd_tests/tests/test_api.py#L73

These queries don't return anything because the Project query is looking for project with slug='pip'. So should I update any of these tests?

[benjaoming]

Hi @tanvimoharir !

The Version object should have slug="0.8", but there should be only 1 Project created for Pip with slug="pip".

[tanvimoharir]

Hi @tanvimoharir !

The Version object should have slug="0.8", but there should be only 1 Project created for Pip with slug="pip".

If you check the above comment with the Django shell, I have only created 1 project object with slug='pip'. However the Version object is getting created automatically with slug='latest'. Maybe I need to explicitly update the version object?

[benjaoming]

In your output, I see:

In [32]: Project.objects.get_or_create(
    ...:             name="Pip",
    ...:             slug="0.8",
    ...:             repo="https://github.com/pypa/pip",
    ...:             repo_type="git",
    ...:         )

The slug is wrong?

[tanvimoharir]

In your output, I see:

In [32]: Project.objects.get_or_create(
    ...:             name="Pip",
    ...:             slug="0.8",
    ...:             repo="https://github.com/pypa/pip",
    ...:             repo_type="git",
    ...:         )

The slug is wrong?

No. So if I create the Project object with slug="0.8" the tests are failing because they are querying the Projects with slug="pip", however Version objects are getting created automatically so I'm unable to understand how to create a project with slug=pip and version_slug="0.8". I will look into the models carefully, I might be missing something.

[benjaoming]

Hi @tanvimoharir

I can see have understood the relation between Projects and Versions well 👍 Great! I was wondering if you can take the current code and instead turn it into an iteration over a dictionary of projects and slugs?

I drafted the idea here: #9411 (comment)

Comment as to why I think that's necessary: In order to have good fixtures, they should be easy to read and maintain. By encoding them in a combined dictionary, we truly have fixtures as data. But once we start to create all these projects and their versions in a long Python code segment, the maintenance and development highly risks getting "stuck" again -- as it was in the previous situation.

P.S. Sorry about not coming back to the previous comment, but I'm glad you managed!

[tanvimoharir]

Hi @tanvimoharir

I can see have understood the relation between Projects and Versions well 👍 Great! I was wondering if you can take the current code and instead turn it into an iteration over a dictionary of projects and slugs?

Sure @benjaoming, I will do that once I get all the tests to pass.

[benjaoming]

That's a great approach!

[tanvimoharir]

Hi @benjaoming, There are now 8 failing tests, I have taken care of all the other errors and failures except these.
I'm unable to figure out these and need some help or if you could point me in the right direction that would be great!
I am moving this to ready for review.

[benjaoming]

@tanvimoharir your timing is most unfortunate :) Today, Tox 4 was released and I'm just in the middle of troubleshooting current build errors. Once #9789 or another similar PR is merged, please merge the main branch into your PR.

[tanvimoharir]

@tanvimoharir your timing is most unfortunate :) Today, Tox 4 was released and I'm just in the middle of troubleshooting current build errors. Once #9789 or another similar PR is merged, please merge the main branch into your PR.

Hi @benjaoming, yeah apologies for the delay. Sure I will merge the main branch as you said. However, the failures I mentioned are because of failing unit tests.
Refer to the earlier build failure: https://app.circleci.com/pipelines/github/readthedocs/readthedocs.org/6055/workflows/c8601cc1-ee04-4355-9a48-c6c216c9e76c/jobs/14661

[benjaoming]

@tanvimoharir I think you need to make eric the owner everywhere that you see that the eric.json fixture has been loaded.

What you are doing in readthedocs/rtd_tests/tests/test_api.py looks correct to me: https://github.com/readthedocs/readthedocs.org/pull/9411/files#diff-106df10243b8ee30687fc30386f521812161a5a963ff9a49dd603b515efd4927R2386

(also the tests from readthedocs/rtd_tests/tests/test_api.py are all passing!)

[benjaoming]

@tanvimoharir you will need to merge in the latest main because there are changes in Tox and CI configurations. Sorry for the inconvenience 👍

[tanvimoharir]

@tanvimoharir you will need to merge in the latest main because there are changes in Tox and CI configurations. Sorry for the inconvenience 👍

Yeah, I did that now. I have also changed the structure of fixtures data to be list of dictionaries as per your comment above. Although I have separated the project list from version list to match what was used in the fixtures json file.
There could still be some pre-commit or formatting related failures which I will take care of.

[tanvimoharir]

I had to add this fixture file (even though it was not part of the original change request)
The reason being that this fixture was dependent on projects data but was loaded before the call_command so I changed this. Refer

[benjaoming]

Note for this PR: After it's merged, we should probably update common/dockerfiles/entrypoints/web.sh to call fixtures_projects.

@tanvimoharir will get back to this PR soon so we can get it wrapped up. You've done a lot of great work here and I don't see any unaddressed feedback. I was just reminded about it after bootstrapping a development environment.

[tanvimoharir]

Note for this PR: After it's merged, we should probably update common/dockerfiles/entrypoints/web.sh to call fixtures_projects.

@tanvimoharir will get back to this PR soon so we can get it wrapped up. You've done a lot of great work here and I don't see any unaddressed feedback. I was just reminded about it after bootstrapping a development environment.

Hi @benjaoming , I have the update at readthedocs/common#153
Regarding this PR, I will get to the formatting related issues which are mostly tox related but the issue with CI build failure is something I'm unable to reproduce. Will check it again!

[benjaoming]

@tanvimoharir you should pull in the latest main branch, the current Circle CI failures have been fixed.

[benjaoming]

@tanvimoharir can you run pre-commit again? The configuration of pre-commit is located in the common submodule, hence you should be using the latest main branch (or pull latest main into the branch you currently have checked out).

# update common
cd common
git checkout main
git pull <readthedocs.org-remote-name> main
cd ..

# re-run pre-commit on files changed compared to your local `main` branch
pre-commit run --from-ref main --to-ref HEAD

[tanvimoharir]

@tanvimoharir can you run pre-commit again? The configuration of pre-commit is located in the common submodule, hence you should be using the latest main branch (or pull latest main into the branch you currently have checked out).

# update common
cd common
git checkout main
git pull <readthedocs.org-remote-name> main
cd ..

# re-run pre-commit on files changed compared to your local `main` branch
pre-commit run --from-ref main --to-ref HEAD

Oh okay, I just ran the pre-commit without updating common's main. Ill do that!

[benjaoming]

@tanvimoharir I see that you ran it and fixes were applied - so I'm not sure how it's not working (this particular pre-commit setup doesn't provide output), might be that your local main branch also needs to be updated?

[tanvimoharir]

@tanvimoharir I see that you ran it and fixes were applied - so I'm not sure how it's not working (this particular pre-commit setup doesn't provide output), might be that your local main branch also needs to be updated?

I did update my local main branch :( (used git pull) Its just saying 'no files to update' and skipping everything now on my local

[tanvimoharir]

I think the problem is that there are some improperly formatted lines apart from the changes ( I did see some places with '' instead of "" and additional whitespace) I will run the pre-commit on the entire repository(pre-commit run --all-files), will that be okay? @benjaoming
the only problem being that this will mess up the diff for this PR :\

[benjaoming]

the only problem being that this will mess up the diff for this PR :\

@tanvimoharir yup, it can be very disruptive to lint the entire repo. If pre-commit changes files, you should only commit the ones that are already part if this PR.

[tanvimoharir]

the only problem being that this will mess up the diff for this PR :\

@tanvimoharir yup, it can be very disruptive to lint the entire repo. If pre-commit changes files, you should only commit the ones that are already part if this PR.

Are you suggesting that I lint the entire repo but only commit the files which are relevant to this PR's changes?

[benjaoming]

that might work, depending on the amount of changes.

[benjaoming]

You can also pull in the changes from #9893 and then you'll see what exactly the problem is...

[stsewd]

@tanvimoharir you can run this command pre-commit run --from-ref main --to-ref HEAD, that will run pre-commit only on the files that have changed from main.

[benjaoming]

@tanvimoharir I might be facing a similar issue now because I'm also running a fork of readthedocs.org. The problem might be that the main branch of your fork is outdated! As weird as it sounds, then currently the pre-commit command that is failing the checks ONLY checks code that was modified compared to the main branch. I think it compares to the main branch of the PR's origin repo - and because that is outdated*, then we get new linting rules applied to old changes.

All of this is unrelated to this PR.

Simply push the latest upstream main branch to the main branch of tanvimoharir/readthedocs.org and then we can re-run and see if that works?

[tanvimoharir]

@tanvimoharir I might be facing a similar issue now because I'm also running a fork of readthedocs.org. The problem might be that the main branch of your fork is outdated! As weird as it sounds, then currently the pre-commit command that is failing the checks ONLY checks code that was modified compared to the main branch. I think it compares to the main branch of the PR's origin repo - and because that is outdated*, then we get new linting rules applied to old changes.

All of this is unrelated to this PR.

Simply push the latest upstream main branch to the main branch of tanvimoharir/readthedocs.org and then we can re-run and see if that works?

Hi @benjaoming , I updated my fork and executed pre-commit run --from-ref main --to-ref HEAD but it just skipped everything.

[benjaoming]

Sorry, I didn't notice what was going on here.

Your PR branch is called main, I'm not quite sure what the result will be here.

Would you be able to fork your current main branch to a new branch and then create a new PR?

Here is a draft of what I mean but please adjust to your needs...

# Go to your local main branch (where the current PR work is)
git checkout main
# Copy it to a new branch called "fixtures"
git checkout -b fixtures
# Push the branch
git push origin fixtures

# Check that it went well and that you have "fixtures" in your repo

# Now let's delete the main branch - this whole thing is dangerous
# on a production project, so please don't use it for any other purposes :)

# Delete the local main branch
git branch -D main

# Assuming the readthedocs/readthedocs.org repo is called "upstream", we get the correct main
git fetch upstream

# Create a headless checkout
git checkout upstream/main

# Create a local branch from the headless checkout
git checkout -b main

# This is the dangerous part -- it force-pushes your local "main" branch
# to overwrite the main branch in tanvimoharir/readthedocs.org
git push -f origin main


# NOW! Finally, please open a new PR using the "fixtures" branch and we
# can start from fresh :)