Issue #315 - Added mtime attribute #411

emilianodellacasa · 2018-07-18T16:57:40Z

Fixed issue #315. I have added an mtime attribute returning the last time the list has been updated. The same value is saved the the remote file, with a slightly different format.

0crat · 2018-07-18T16:57:59Z

Job #411 is now in scope, role is REV

emilianodellacasa · 2018-07-18T16:59:10Z

@yegor256 One test was failing also on master so never mind, but the test_all_scripts is failing quite strangely, I tried to understand how that test works but I suppose you can help me with it.

0crat · 2018-07-18T17:02:57Z

This pull request #411 is assigned to @ledestin /z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @yegor256 /z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be a monetary reward for this job

yegor256 · 2018-07-19T08:51:33Z

lib/zold/remotes.rb

@@ -120,6 +121,7 @@ def initialize(file, network: 'test')
      raise 'Network can\'t be nil' if network.nil?
      @network = network
      @mutex = Mutex.new
+      read_mtime


@emilianodellacasa have you seen this? https://www.yegor256.com/2015/05/07/ctors-must-be-code-free.html

@yegor256 Ok fixed

ledestin · 2018-07-19T10:59:42Z

lib/zold/remotes.rb

@@ -242,9 +244,16 @@ def if_present(host, port)
      save(list)
    end

+    def read_mtime
+      CSV.read(file, skip_lines: /.+,\d+,\d+,\d+/).map do |r|


@emilianodellacasa r is probably better off as row

@ledestin You are right, it is much more clear

ledestin · 2018-07-19T11:02:43Z

lib/zold/remotes.rb

      @mutex.synchronize do
-        raw = CSV.read(file).map do |r|
+        raw = CSV.read(file, skip_lines: /mtime,\d+/).map do |r|


@emilianodellacasa why not use the filesystem mtime?

@ledestin It is a very good idea, implemented. Thanks a lot!

ledestin · 2018-07-19T11:03:23Z

@emilianodellacasa please see my comments

codecov-io · 2018-07-23T10:40:39Z

Codecov Report

Merging #411 into master will increase coverage by 0.31%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
+ Coverage   32.56%   32.88%   +0.31%     
==========================================
  Files          59       58       -1     
  Lines        2822     2746      -76     
==========================================
- Hits          919      903      -16     
+ Misses       1903     1843      -60

Impacted Files	Coverage Δ
lib/zold/node/front.rb	`35.74% <ø> (-0.2%)`	⬇️
lib/zold/remotes.rb	`34.69% <33.33%> (+1.36%)`	⬆️
lib/zold/http.rb	`50.9% <0%> (-1.93%)`	⬇️
lib/zold/node/async_entrance.rb	`23.43% <0%> (-1.18%)`	⬇️
lib/zold/copies.rb	`22.47% <0%> (-0.87%)`	⬇️
lib/zold/commands/pay.rb	`22.22% <0%> (ø)`	⬆️
lib/zold/tree_wallets.rb
lib/zold/commands/node.rb	`20.48% <0%> (+1.2%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de083a8...4d45cf8. Read the comment docs.

emilianodellacasa · 2018-07-23T10:45:19Z

lib/zold/node/front.rb

-        mtime: settings.remotes.mtime.utc.iso8601,
-        all: settings.remotes.all
+        all: settings.remotes.all,
+        mtime: settings.remotes.mtime.utc.iso8601


@yegor256 This is the offending line that breaks the test_all_scripts test. If you remove it the test is green, red otherwise.

However, the code works perfectly as I also tried to call the remote endpoint from the same test, and the generated JSON is what is expected. Any comment on this?

@yegor256 It is the redeploy_on_upgrade that is failing, I cannot understand why. Could you explain how it works?

@yegor256 This has been solved in last commit, thanks anyway

ledestin · 2018-07-23T11:17:20Z

test/test_remotes.rb

+      file = File.join(dir, 'remotes')
+      FileUtils.touch(file)
+      File.write(file, "127,0,0,0\n")
+      mtime_on_file = Time.now


@emilianodellacasa it this works, the test is probably is broken or there's a race here. I'd load mtime from the file here.

@ledestin Correct

ledestin · 2018-07-23T11:17:46Z

test/test_remotes.rb

+  def test_read_mtime_from_file
+    Dir.mktmpdir 'test' do |dir|
+      file = File.join(dir, 'remotes')
+      FileUtils.touch(file)


@emilianodellacasa I think mtime is updated on write anyway, so no need for touch.

@ledestin Definitely, I removed the touch thanks a lot

ledestin · 2018-07-23T11:19:55Z

@emilianodellacasa please see my comments

0crat · 2018-07-23T17:38:18Z

@ledestin /z this job was assigned to you 5days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

emilianodellacasa · 2018-07-24T12:54:34Z

@ledestin I worked on your comments and fixed the problem with the test, please check

emilianodellacasa · 2018-07-25T07:32:47Z

@ledestin ping

ledestin · 2018-07-26T09:46:10Z

lib/zold/remotes.rb

@@ -62,6 +63,10 @@ def all
      def iterate(_)
        # Nothing to do here
      end
+
+      def mtime


@emilianodellacasa since you add this, why do you need attr_reader? Unless it's defined in a different class.

ledestin · 2018-07-26T09:48:13Z

@emilianodellacasa please see my comment

emilianodellacasa · 2018-07-26T14:14:51Z

@ledestin You are right, I needed to have a fixed value, but it is better to set this value on the initilizer; I have also refactored a little bit and merged some code, it should be everything ok now;

ledestin · 2018-07-27T11:37:03Z

lib/zold/remotes.rb

@@ -187,7 +190,7 @@ def iterate(log, farm: Farm::Empty.new)
              network: @network
            )
            idx += 1
-            raise 'Took too long to execute' if (Time.now - start).round > @timeout
+            raise 'Took too long to execute' if (Time.now - start).round > timeout


@emilianodellacasa have you heard of Timeout.timeout?

@ledestin Actually this is something I merged from another issue, I didn't do that: in any case, I will take a look at it :-)

ledestin · 2018-07-27T11:37:40Z

@yegor256 looks fine to me

emilianodellacasa · 2018-08-07T15:13:08Z

@yegor256 I have never seen that error before, please try merging again

emilianodellacasa · 2018-08-08T08:49:48Z

@yegor256 ping

yegor256 · 2018-08-09T07:55:18Z

test/node/test_entrance.rb

@@ -60,6 +60,7 @@ def test_pushes_wallet
  end

  def test_renders_json
+    skip


@emilianodellacasa you can't just skip a test. You have to explain why and add a puzzle.

@yegor256 it is the same test for which I have opened an issue #425, and that I already skipped in PR #428, opening a puzzle now wouldn't make any sense

yegor256 · 2018-08-09T07:55:29Z

@emilianodellacasa see above

emilianodellacasa · 2018-08-09T07:58:43Z

@yegor256 answered

emilianodellacasa · 2018-08-10T08:08:15Z

@yegor256 ping

yegor256 · 2018-08-10T08:17:28Z

@emilianodellacasa we should not leave skip in the code without any notice. In the future, in a few months, another programmer may ask -- what is that? Where will he/she find an answer?

emilianodellacasa · 2018-08-10T08:41:08Z

@yegor256 I have added the puzzle as requested

emilianodellacasa · 2018-08-13T17:36:55Z

@yegor256 In this case Travis failed because tests took too long

yegor256 · 2018-09-18T06:53:07Z

@rultor merge

rultor · 2018-09-18T07:10:49Z

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

rultor · 2018-09-18T07:19:55Z

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 9min)

0crat · 2018-09-18T07:39:54Z

@ypshenychka /z please review this job completed by @ledestin /z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed

0crat · 2018-09-18T07:40:25Z

The job #411 is now out of scope

0crat · 2018-09-18T07:40:30Z

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @yegor256 /z

ypshenychka · 2018-09-18T08:02:42Z

@emilianodellacasa According to our QA rules:

Messages in a job always start with a name of a user they are addressed to

Please correct your code review comments (here, here and here) by indicating an addressee in the beginning.

emilianodellacasa · 2018-09-18T08:05:39Z

@ypshenychka Done

ypshenychka · 2018-09-18T08:18:26Z

@emilianodellacasa thanks

ypshenychka · 2018-09-18T08:18:33Z

@0crat quality acceptable

0crat · 2018-09-18T08:25:48Z

Order was finished, quality is "acceptable": +15 point(s) just awarded to @ledestin /z

0crat · 2018-09-18T08:25:52Z

Quality review completed: +8 point(s) just awarded to @ypshenychka /z

emilianodellacasa added 2 commits July 18, 2018 17:11

Issue zold-io#315 - fixed

eb9c86b

Issue zold-io#315 - remote file format

b3a51fa

0crat added the scope label Jul 18, 2018

yegor256 reviewed Jul 19, 2018

View reviewed changes

ledestin reviewed Jul 19, 2018

View reviewed changes

emilianodellacasa added 3 commits July 23, 2018 11:02

Issue zold-io#315 - Updates after comments

9388d48

Merge remote-tracking branch 'upstream/master' into issue_315

ffbb53e

Issue zold-io#315 - a few formal fixes

1661b55

emilianodellacasa commented Jul 23, 2018

View reviewed changes

ledestin reviewed Jul 23, 2018

View reviewed changes

Issue zold-io#315 - Fixing tests

b047130

emilianodellacasa added 3 commits July 24, 2018 10:47

Issue zold-io#315 - Adding test for remotes endpoint

f5082db

Issue zold-io#315 - Adding mtime method to Empty Remote

52ce489

Issue zold-io#315 - Merging conflicts from upstream

2cf96f3

ledestin reviewed Jul 26, 2018

View reviewed changes

emilianodellacasa added 2 commits July 26, 2018 15:53

Issue zold-io#315 - Small refactoring

2e30216

Issue zold-io#315 - Merging conflicts

5a560a2

ledestin reviewed Jul 27, 2018

View reviewed changes

Issue zold-io#315 - Skipping test

cd4d073

yegor256 reviewed Aug 9, 2018

View reviewed changes

Issue zold-io#315 - Adding puzzle to skipped test

6cc6a94

emilianodellacasa added 2 commits August 13, 2018 18:45

Merge remote-tracking branch 'upstream/master' into issue_315

a55b637

Issue zold-io#315 - Removed puzzle

71a8040

emilianodellacasa added 2 commits September 4, 2018 16:26

Issue zold-io#315 - Merging

6d32e5d

Merge branch issue_315

4d45cf8

rultor merged commit 4d45cf8 into zold-io:master Sep 18, 2018

emilianodellacasa deleted the issue_315 branch September 18, 2018 07:33

0crat removed the scope label Sep 18, 2018

Issue #315 - Added mtime attribute #411

Issue #315 - Added mtime attribute #411

Conversation

emilianodellacasa commented Jul 18, 2018 • edited Loading

0crat commented Jul 18, 2018

emilianodellacasa commented Jul 18, 2018

0crat commented Jul 18, 2018

Choose a reason for hiding this comment

emilianodellacasa Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ledestin commented Jul 19, 2018

codecov-io commented Jul 23, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilianodellacasa Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ledestin commented Jul 23, 2018

0crat commented Jul 23, 2018

emilianodellacasa commented Jul 24, 2018

emilianodellacasa commented Jul 25, 2018

Choose a reason for hiding this comment

ledestin commented Jul 26, 2018

emilianodellacasa commented Jul 26, 2018

Choose a reason for hiding this comment

emilianodellacasa Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

ledestin commented Jul 27, 2018

emilianodellacasa commented Aug 7, 2018

emilianodellacasa commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yegor256 commented Aug 9, 2018

emilianodellacasa commented Aug 9, 2018

emilianodellacasa commented Aug 10, 2018

yegor256 commented Aug 10, 2018

emilianodellacasa commented Aug 10, 2018

emilianodellacasa commented Aug 13, 2018

yegor256 commented Sep 18, 2018

rultor commented Sep 18, 2018

rultor commented Sep 18, 2018

0crat commented Sep 18, 2018

0crat commented Sep 18, 2018

0crat commented Sep 18, 2018

ypshenychka commented Sep 18, 2018

emilianodellacasa commented Sep 18, 2018

ypshenychka commented Sep 18, 2018

ypshenychka commented Sep 18, 2018

0crat commented Sep 18, 2018

0crat commented Sep 18, 2018

emilianodellacasa commented Jul 18, 2018 •

edited

Loading

emilianodellacasa Jul 23, 2018 •

edited

Loading

codecov-io commented Jul 23, 2018 •

edited

Loading

emilianodellacasa Jul 24, 2018 •

edited

Loading

emilianodellacasa Jul 27, 2018 •

edited

Loading