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

The correction of how the 'error_count' is calculated in 'flags['update_logs']'. #5673

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Joice-crypto
Copy link
Contributor

What this PR does

This PR resolves the bug from issue #5580.
This PR fixes the bug; now the 'error_count' in 'flags['update_logs']' contains the total error count instead of accumulated errors.

Screenshots

Before:
error_countA

After:
error_countD

lib/replica.rb Outdated
@@ -148,7 +148,7 @@ def api_post(endpoint, key, data)
rescue StandardError => e
tries -= 1
sleep 2 && retry unless tries.zero?
log_error(e, update_service: @update_service,
log_error(e, update_service: @update_service,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the only change here is to add a blank space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I didn't make any changes to this file

Copy link
Member

Choose a reason for hiding this comment

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

Please revert the changes then, as you did for schema.rb file.

db/schema.rb Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This change shouldn't be committed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, should I remove this file from commit?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I don't think this PR should modify schema.rb file at all.

@gabina
Copy link
Member

gabina commented Feb 28, 2024

CI is failing due to ruby linting. Please fix that running rubocop locally.

@Joice-crypto
Copy link
Contributor Author

CI is failing due to ruby linting. Please fix that running rubocop locally.

I fixed the files that were causing problems , but the error is still appearing in this file: spec/features/article_viewer_spec.rb:24:101: C: Layout/LineLength: Line is too long. [118/100]. But I didn't change this file and locally when I ran rubocop no errors appeared.

@gabina
Copy link
Member

gabina commented Feb 29, 2024

You still have to revert some unwanted changes, such as removing a line in app/services/update_course_stats.rb. Please check all the "files changed" and revert all the changes that are not meaningful for this pull request.

@Joice-crypto
Copy link
Contributor Author

You still have to revert some unwanted changes, such as removing a line in app/services/update_course_stats.rb. Please check all the "files changed" and revert all the changes that are not meaningful for this pull request.

I did this. Now it's correct?

Comment on lines +359 to +360
nokogiri (1.15.4-x86_64-linux)
racc (~> 1.4)
Copy link
Member

Choose a reason for hiding this comment

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

Why are these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was receiving the following error: Error: The process '/opt/hostedtoolcache/Ruby/3.1.2/x64/bin/bundle', so as a solution, I ran bundle lock --add-platform x86_64-linux

Copy link
Member

Choose a reason for hiding this comment

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

What error were you receiving?

Error: The process '/opt/hostedtoolcache/Ruby/3.1.2/x64/bin/bundle' doesn't look like a complete error.
Anyway, I don't think you should add these changes in your PR, as the error is probably due to a problem with your local environment, I guess.

@@ -605,6 +607,7 @@ GEM

PLATFORMS
ruby
x86_64-linux
Copy link
Member

Choose a reason for hiding this comment

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

Same question here

@@ -128,7 +129,7 @@
subject
end
sentry_tag_uuid = subject.sentry_tag_uuid
expect(course.flags['update_logs'][1]['error_count']).to be_positive
expect(course.flags['update_logs'][1]['error_count']).to be >= 0
Copy link
Member

Choose a reason for hiding this comment

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

Why changing be_positive to >=0?

@@ -87,7 +87,7 @@
subject
end
sentry_tag_uuid = subject.sentry_tag_uuid
expect(course.flags['update_logs'][1]['error_count']).to eq 1
expect(course.flags['update_logs'][1]['error_count']).to eq 0
Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate on what is this spec for and why it should be changed to 0?

@@ -106,7 +106,8 @@
subject
end
sentry_tag_uuid = subject.sentry_tag_uuid
expect(course.flags['update_logs'][1]['error_count']).to eq 8
error_count = subject.error_count
Copy link
Member

Choose a reason for hiding this comment

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

What is subject.error_count?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, the expected value was 1 because @error_count = error_count + 1, but I changed it, and error_count is now not incremented by 1, but by the value of new_errors, which can be 0. So, I set expect(course.flags['update_logs'][1]['error_count']).to eq 0.

It's the same case for expect(course.flags['update_logs'][1]['error_count']).to be >= 0. I used >=0 because I was receiving the value of new_errors as 0, so I set it to accept values equal to or greater than 0.

In expect(course.flags['update_logs'][1]['error_count']).to eq error_count, instead of 8, I changed it to be equal to the value of the variable error_count, which comes from error_count = subject.error_count, which is fetching the error values.

Copy link
Member

Choose a reason for hiding this comment

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

Specs should try to demonstrate that the count of errors is correct. It doesn't make sense to write only specs with error_count = 0. You should think better examples to exercise the new counting of errors.

@new_errors_count ||= 0
end

def new_errors_count=(value)
Copy link
Member

Choose a reason for hiding this comment

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

is that = a typo?

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