readthedocs.org

readthedocs.org copied to clipboard

Refers #9332

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

• 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 :+1:

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.

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!

@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.

Hi @tanvimoharir :wave:

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.

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?

Hi @tanvimoharir !

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

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?

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?

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.

Hi @tanvimoharir

I can see have understood the relation between Projects and Versions well :+1: 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: https://github.com/readthedocs/readthedocs.org/pull/9411#discussion_r985753107

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!

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.

That's a great approach!

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.

@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 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

@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!)

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

@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.

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.

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 https://github.com/readthedocs/common/pull/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!

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

@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 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!

@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 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

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 :\

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.

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?

that might work, depending on the amount of changes.