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

fix ob-async does not work with :results file link based types. #52

Merged
merged 1 commit into from
Feb 20, 2019
Merged

fix ob-async does not work with :results file link based types. #52

merged 1 commit into from
Feb 20, 2019

Conversation

stardiviner
Copy link
Contributor

No description provided.

@astahlman
Copy link
Owner

🎉 Thanks @stardiviner! I'll take a look at the PR this weekend and merge if it looks OK

Copy link
Owner

@astahlman astahlman left a comment

Choose a reason for hiding this comment

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

Could you add a unit test for this, as well?

ob-async.el Outdated Show resolved Hide resolved
@stardiviner
Copy link
Contributor Author

I write a test:

modified   test/ob-async-test.el
@@ -211,6 +211,26 @@ when content has been added below the source block"
                (let ((foo-contents (progn (find-file "/tmp/foo") (buffer-substring-no-properties (point-min) (point-max)))))
                  (should (string= "Don't wait on me\n" foo-contents))))))))
 
+(ert-deftest test-async-execute-file-block ()
+  "Test that we can insert results when header-arg :file is present"
+  (let ((buffer-contents "Here's a sh source block:
+
+  #+BEGIN_SRC sh :async :results link :file \"/tmp/foo\"
+     echo \"Don't wait on me\" > /tmp/foo
+  #+END_SRC"))
+    (with-buffer-contents
+     buffer-contents
+     (org-babel-next-src-block)
+     (ctrl-c-ctrl-c-with-callbacks
+      :pre (should (placeholder-p (results-block-contents)))
+      :post (progn
+              (should (string= "[[file:/tmp/foo]]" (results-block-contents)))
+              (let ((foo-contents (progn
+                                    (find-file "/tmp/foo")
+                                    (buffer-substring-no-properties
+                                     (point-min) (point-max)))))
+                (should (string= "Don't wait on me\n" foo-contents))))))))
+
 (ert-deftest test-async-execute-table-output ()
   "Test that we can insert table output"
   (let ((buffer-contents "Here's a source block:

But run make test get all tests failed:

Ran 18 tests in 325.615 seconds
1 expected failures
15 unexpected results:
   FAILED  test-async-ctrl-c-ctrl-c-hook
   FAILED  test-async-execute-call
   FAILED  test-async-execute-existing-sh-block
   FAILED  test-async-execute-file-block
   FAILED  test-async-execute-fresh-sh-block
   FAILED  test-async-execute-named-block
   FAILED  test-async-execute-named-block-with-results
   FAILED  test-async-execute-named-call-block
   FAILED  test-async-execute-python-block
   FAILED  test-async-execute-table-output
   FAILED  test-async-execute-tramp-block
   FAILED  test-async-return-to-point-above-block
   FAILED  test-async-return-to-point-below-block
   FAILED  test-output-to-file-with-dir
   FAILED  test-pre-execute-hook
make: *** [Makefile:19: test] Error 1

This is weird.

@stardiviner
Copy link
Contributor Author

Here is part of backtrace: https://gist.github.com/d5da758eca42a55df084fea5824cd73b

@astahlman astahlman closed this Feb 19, 2019
@astahlman astahlman reopened this Feb 19, 2019
@astahlman astahlman closed this Feb 19, 2019
@astahlman astahlman reopened this Feb 19, 2019
@astahlman
Copy link
Owner

@stardiviner I think your PR should be passing tests now, but since you force-pushed Travis seems to be in a weird state and isn't running against your latest commit. Not sure what's going on there - maybe try opening a new Pull Request?

@stardiviner
Copy link
Contributor Author

Seems force push can't re-trigger Travis CI. Is that need to re-open a new PR? Ok, I will.

@stardiviner
Copy link
Contributor Author

Ok, I tried to create a new PR with same you and mine master branch comparing. It does not have the new PR button. Only have view this PR button. So ....

@stardiviner
Copy link
Contributor Author

I checked out Travis CI log, two error reports are failed on make install-dev command.

The command "make install-dev" failed and exited with 2 during .

(ctrl-c-ctrl-c-with-callbacks
:pre (should (placeholder-p (results-block-contents)))
:post (progn
(should (string= "[[file:/tmp/foo]]" (results-block-contents)))
Copy link
Owner

Choose a reason for hiding this comment

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

This should just be (string= "/tmp/foo"). The raw text is "[[file:/tmp/foo]]", but (results-block-contents) calls (org-babel-read-results), which turns [[file:/tmp/foo]] into /tmp/foo

@astahlman
Copy link
Owner

That last error looks like a transient error fetching from elpa. I just re-ran the tests, and this time the error looks real:

Test test-async-execute-file-block condition:
    (ert-test-failed
     ((should
       (string= "[[file:/tmp/foo]]"
		(results-block-contents)))
      :form
      (string= "[[file:/tmp/foo]]" "/tmp/foo")
      :value nil))

I think you can fix it by changing "[[file:/tmp/foo]]" to "/tmp/foo". No need to rebase and force-push when you make the change - I can squash your commits upon merging.

@stardiviner
Copy link
Contributor Author

Ok, I force pushed update. Thanks fore reviewing, @astahlman .

@astahlman
Copy link
Owner

@stardiviner I didn't have permission to push to your branch, so I created a throwaway PR and confirmed that it fixes the test: https://github.com/astahlman/ob-async/pull/54/files#r258313059

Want to apply that change to your branch so we can merge this one? (It's a one-line change).

@stardiviner
Copy link
Contributor Author

stardiviner commented Feb 20, 2019

Ok, I added it now, force pushed. Seems trigger Tracis CI again now.

@stardiviner
Copy link
Contributor Author

stardiviner commented Feb 20, 2019

Wow, all passed, clean green now. THanks @astahlman

@astahlman
Copy link
Owner

🎉 🎉 🎉

@astahlman astahlman merged commit 73e57a9 into astahlman:master Feb 20, 2019
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