Skip to content

Conversation

unroma
Copy link

@unroma unroma commented Aug 28, 2025

Issue being resolved:

#345
#244
#287
#127

Most probably solved:

#332
#340

Solution description

Caution! A lot of text below!
Current solution is not completed. Work in progress! It's not even tested properly and what is very important unit tests are not written/updated. So, now existing unit test will fail, no doubts about that. I' m going to update description time to time. Keep in mind that this text is not final.
For now I created new packages with the names of old and ending _v2 . When I finish the old packaged must be replaced.
I'm trying to leave some my thoughts and comments in code (later might be removed)

What was already done:

  • Refactored many methods and classes
  • Reduced number of parameter in constructors
  • Reduced return parameters number in methods.
  • Removed complex nested ifs
  • Reworked error handling
  • Reworked roll back methods.
  • Changed access modifiers for fields
  • Reworked data provider class
  • I used composition to split logic in ApiRequestHandler, added new package with api helpers classes
  • In some api helpers classes implemented kind of cache mechanism.
  • Moved method create_or_update_test_run to ResultsUploader class (seems it's more logical place for it) (still TODO probably define more public methods, like update cases only or add only new. )
  • Reworked case_automatio_id way to identify system name and update upload.
  • Added subsection support
  • Added case update support

Changes

I will top up this step by step:

  • Slightly changed logic of resolving project. Previous solution was trying to resolve by name and if no success then by provided id. I suppose if id of project provided, then it must be resolved by id, not use hidden logic with name. When someone specifies id, he knows what exact project must be used
  • Removed empty section deletion from ResultsUploader. It's redundant. Junit parser didn't map empty sections and subsections. (and this is parser's responsibility). There is a case when empty section might be created:
  1. suite has new case(s) to add
  2. suite has existing case(s) in new section (only existing), but update is not allowed.
    In that case section will be created, but I think this is expected behavior. It's possible to handle add prohibit creation, but I don't think is necessary and only will make logic more complicated.
  • Removed section names duplication check. It's not necessary. TestRail supports duplicated names, and anyway the first matched section with name will be selected, rest stuff is not upload classes responsibility. If need must be validated or merged at parser level.
  • instantiate_api_client method moved from ProjectBasedClient to more appropriate place in api helpers package

Potential impacts

Many places, hard to say right now, will try to point later, together with unit tests

Steps to test

Currently hard. Only with replacements in imports.
Work still in progress. Also would be nice to have discussion with interested people form community.
I performed some common tests and looks like sulution works. Any way i will continue working on tests.

PR Tasks

  • PR reference added to issue
  • README updated
  • Unit tests added/updated

@unroma unroma changed the title Refactor/major refactoring Refactor/major refactoring WIP Aug 28, 2025
@unroma unroma changed the title Refactor/major refactoring WIP Major refactoring WIP Aug 28, 2025
@acuanico-tr-galt
Copy link
Collaborator

Hi @unroma we have created a new refactor branch: refactor/1.11.0 which is based on version 1.11.0. We will be using this branch to move or rebase refactored code gradually as we also test them. Please point this PR to the said branch. Thank you :)

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