Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add VerifyTests #86

Merged
merged 37 commits into from
May 12, 2024
Merged

Conversation

rstm-sf
Copy link
Contributor

@rstm-sf rstm-sf commented Nov 25, 2022

@rstm-sf rstm-sf force-pushed the feature/verify_tests branch 2 times, most recently from b268a57 to 5997d8e Compare November 27, 2022 10:34
@rstm-sf rstm-sf force-pushed the feature/verify_tests branch 6 times, most recently from 1774830 to 90b70ad Compare December 3, 2022 14:29
@rstm-sf
Copy link
Contributor Author

rstm-sf commented Dec 3, 2022

@PrzemyslawKlys hello!

we need to add more tests for cases like this,, to check order of paragraphs objects
#82 (comment)

I decided to golden tests to make it easier to do refactoring https://youtu.be/wA7oJDyvn4c

Is it possible to change the name of the pipeline?)
image

@PrzemyslawKlys
Copy link
Member

Hi,

I saw you playing with this, and this is an excellent idea. You will need to add some "how to" so it can be easily regenerated if needed so I can do it myself from time to time ;)

As for the name of the pipeline I am not sure where it's coming from:

image

Do you know where?

approve-all.ps1 Outdated Show resolved Hide resolved
@PrzemyslawKlys
Copy link
Member

I've renamed the main pipeline. I guess it should work

image

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Nov 6, 2023

Ok. It's done

Sorry for the delay, but I haven’t yet decided how to find the reason why the test breaks. But everything seems to be fine now (at least they passed several tests in a row). If the AdvancedDocumentTests.AdvancedWordCreate test flickers, you will have to skip it again and create a problem. Can tag me)

Also, I did not add all possible options to the tests. I think this is a good start

@rstm-sf rstm-sf marked this pull request as ready for review November 6, 2023 07:51
@PrzemyslawKlys
Copy link
Member

Hi,

Couple of questions:

  • Do we need those csproj changes?
  • I saw you did remove a lot from gitignore? Not needed? Seems a lot got removed?
  • Can't we reuse those tests we already have and simply add a line that is responsible for generation?

I'm looking for number of line commits and it's 170k. If that has to happen with every change to library that seems like a lot of commits mostly because of .txt.

I wonder if we're able to somehow solve it differently?

Thank you,
Przemek

@PrzemyslawKlys
Copy link
Member

I guess change to Azure Devops will break this table?

image

As it's no longer separate, so we won;t be able to tell?

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Nov 6, 2023

Do we need those csproj changes?

Do you mean end of file? The current file the config, think the IDE fixed it itself https://github.com/EvotecIT/OfficeIMO/blob/2cf68efae04b4917ea911923a7e450463b8d4eab/.editorconfig#L5C1-L5C21

I saw you did remove a lot from gitignore? Not needed? Seems a lot got removed?

Sorry, this is personal :) I can return it back. "GitIgnore for the studio" is more suitable for dense legacy...

Can't we reuse those tests we already have and simply add a line that is responsible for generation?

It's possible, but I don't like how the tests are written. They assume that once launched, the application opens to view the result. This no longer looks like a unit test. In addition, there are no options (as far as I remember) that edit existing documents

I'm looking for number of line commits and it's 170k. If that has to happen with every change to library that seems like a lot of commits mostly because of .txt.

I wonder if we're able to somehow solve it differently?

It seems to me that this is true. Thus, we see all the changes that may affect with PR. The big change will be for new files. Well, now many tests have been added at once

For example, look at this commit ca91747

It shows that new styles were added last year, as well as endnotes and footnotes. Also, there are changes to previously generated properties. I think this is a very clear representation of how the file will change after PR

@PrzemyslawKlys
Copy link
Member

Can't we reuse those tests we already have and simply add a line that is responsible for generation?

It's possible, but I don't like how the tests are written. They assume that once launched, the application opens to view the result. This no longer looks like a unit test. In addition, there are no options (as far as I remember) that edit existing documents

Huh? I think we only have that option in Examples, not Tests? And it's always set to False? Not sure I understand?

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Nov 6, 2023

Huh? I think we only have that option in Examples, not Tests? And it's always set to False? Not sure I understand?

Oops, my fail.

But still, I don’t like that different approaches are mixed. If you want them to be in the same file/project, then I will delete them and add them to the existing ones

@PrzemyslawKlys
Copy link
Member

It's fine, just trying to understand, that's all.

@PrzemyslawKlys
Copy link
Member

I was just thinking it would mean for every new test, new file would be created as well, and that would lead to larger coverage, but maybe that's a bit too much and it's better to focus on directly those tests.

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Nov 6, 2023

As it's no longer separate, so we won;t be able to tell?

Sorry, I didn't use azure devops as an administrator. Moreover, I did not generate badges for it. I think it’s worth returning back then (this table is not interesting for me :))

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Apr 22, 2024

Hello @PrzemyslawKlys

Are we going to merge this?)

Here we can see how the document changed during hold time (the tests did not change) d4135d7

@PrzemyslawKlys
Copy link
Member

Yes, we need to commit to this. I've been putting this away a bit, procrastinating because I know that once I commit this, I will have one more step to take care of with each and every PR ;)

@PrzemyslawKlys PrzemyslawKlys merged commit 9491840 into EvotecIT:master May 12, 2024
3 checks passed
@rstm-sf rstm-sf deleted the feature/verify_tests branch May 13, 2024 04:57
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.

None yet

2 participants